Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'Fail_On_All_Warnings' CLI Argument #122

Closed

Conversation

1eyewonder
Copy link
Contributor

Saw #118 and thought I'd take a stab at it. Per #118 (comment), I read through the documentation and implemented what I interpreted. If I misread, let me know and I can refactor accordingly.

@nojaf
Copy link
Contributor

nojaf commented Oct 21, 2023

Hi there! I appreciate your interest.
At first glance, I wonder if we need to shuffle some more.

Maybe sticking to the compiler options, makes more sense here:

WarningLevel / -warn: Set warning level.
AnalysisLevel: Set optional warning level.
TreatWarningsAsErrors / -warnaserror: Treat all warnings as errors
WarningsAsErrors / -warnaserror: Treat one or more warnings as errors
WarningsNotAsErrors / -warnnotaserror: Treat one or more warnings not as errors
NoWarn / -nowarn: Set a list of disabled warnings.
CodeAnalysisRuleSet / -ruleset: Specify a ruleset file that disables specific diagnostics.
ErrorLog / -errorlog: Specify a file to log all compiler and analyzer diagnostics.
ReportAnalyzer / -reportanalyzer: Report additional analyzer information, such as execution time.

@1eyewonder
Copy link
Contributor Author

Is the thought process behind this we have a breaking change to name in this fashion explicitly? Also, are you thinking we want to expand the scope of this issue/PR to try and tackle all the flags listed here?

@nojaf
Copy link
Contributor

nojaf commented Oct 21, 2023

Yes, I think a breaking change is fine as we are still in pre-release.
We broke the API a couple of times these last weeks, so I'm not overly worried about that.
In recent (private) conversations, we found the need to have more fine-grained control so revisiting makes sense.

@dawedawe
Copy link
Contributor

To cover all cases of bending the default Severity level of a specific analyzer message code to another Severity level, we had the idea to provide the following CLI switches:

--TreatAsInfo MessageCode1 MessageCode2 ...
--TreatAsHint MessageCode3 MessageCode4 ...
--TreatAsWarning MessageCode5 MessageCode6 ...
--TreatAsError MessageCode7 MessageCode8 ...

That would make it very easy and clear for the user to state their intent and we already had a use case of users wanting to have a very fine grained control.
We could still offer CLI switches known from the compiler like TreatWarningsAsErrors, but internally map them to the above.
@baronfel @TheAngryByrd What do you think? Would that be okay for you?

@TheAngryByrd
Copy link
Member

Getting into the super fine control there is a few locations to think about.

  • Per line
  • Per region (a section of a file)
  • Per file (we have Ignore_Files but that's a whole sale don't use any analyzers rather than choosing which analyzers to turn on/off per file)
  • Per project

And as you've pointed out there's different ways to treat a MessageCode.

  • TreatAsHint
  • TreatAsInfo
  • TreatAsWarning
  • TreatAsError
  • Disable (similar to NoWarn in msbuild, We currently have Exclude_Analyzer but that's for a whole analyzer and not specific code as an analyzer may have multiple MessageCodes).

For instance it's reasonable for someone to want to ignore Option.value analyzer in a specific file or even line, but not the rest of the project.

This creates a very fun matrix of support cases, some are easier than others. We only cover some of these cases in certain areas but this is the all the places someone is going to want to do some type of configuration.

@nojaf
Copy link
Contributor

nojaf commented Oct 23, 2023

For the locations bit, I think it is fine to stay on the project level until someone has the use case.

We are invested in the treatAs case as it has surfaced at GR.

@baronfel baronfel deleted the branch ionide:master December 12, 2023 18:17
@baronfel baronfel closed this Dec 12, 2023
@baronfel
Copy link
Collaborator

Please rebase this against main to reopen the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants