Conversation
chsienki
force-pushed
the
warning_waves_proposal
branch
from
f0df091 to
cebbe58
Compare
|
|
||
| <!-- Use wave 10.0 to suppress warnings introduced in wave 11.0 --> | ||
| <!-- All existing warnings from wave 10.0 are still reported --> | ||
| <RazorWarningWave>10.0</RazorWarningWave> |
There was a problem hiding this comment.
MSBuild property name is up for debate
There was a problem hiding this comment.
I think RazorWarningLevel to match Roslyn (WarningLevel)
| 2. What is the minimum warning wave version we should support? Are values like 8 valid, or only 10+? | ||
|
|
||
| 3. Should we have a 'Latest' or 'Preview' warning wave value? Seems like if we want to do 1. then we would need these, but not otherwise the omission of the value serves the same purpose. | ||
|
|
There was a problem hiding this comment.
Should have another open question about if we should tie RazorWarningWave to AnalysisLevel by default as discussed in the language design issue. My thought is we probably should.
|
|
||
| ## Open Questions | ||
|
|
||
| 1. Should there be a way to opt-in to future warnings (set wave higher than language version) for early adopters? |
There was a problem hiding this comment.
Sounds useful and simple (in Roslyn, this is done by setting warn level to 9999)
|
|
||
| ## References | ||
|
|
||
| - [C# Warning Waves Documentation](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/warning-waves) |
There was a problem hiding this comment.
| Currently, upgrading to a newer version of .NET brings the corresponding Razor language version by default as the language version is tied to the TFM. When new warnings are introduced in a language version, users with `<TreatWarningsAsErrors>true</TreatWarningsAsErrors>` face a difficult choice: | ||
|
|
||
| 1. **Fix all new warnings immediately** - This can be time-consuming and block upgrades, especially for large codebases. | ||
| 2. **Downgrade the language version** - This allows the upgrade but prevents users from using new language features. | ||
| 3. **Disable specific warnings** - This requires identifying each new warning and suppressing them individually, which is error-prone and may hide legitimate issues. |
There was a problem hiding this comment.
Is the main value prop of the warnings to simplify the bulk disabling of new warnings (which in other circumstances would require one of the three things you mentioned).
If so, would it make sense to have a Latest/Preview value by default (given that new users can now opt-out with a single gesture). As an alternative, it would default to the TFM unless explicitly configured, won't it?
| 4. **Tooling Updates**: Ensure IDE tooling (Visual Studio, VS Code) respects warning wave settings. | ||
| 5. **Documentation**: Document all warnings and their associated waves. | ||
|
|
||
| ## Open Questions |
There was a problem hiding this comment.
Why not simply be consistent with roslyn in all aspects of warning waves?
|
|
||
| ### Warning Wave Versioning | ||
|
|
||
| Warning waves will be versioned to match Razor language versions: |
There was a problem hiding this comment.
I'm not sure we should mention Razor language version, as its essentially only by convention that it matches the .NET version, and that's what we're really matching IMO. Also ties with Roslyn, which definitely doesn't match lang version and warning waves.
|
|
||
| <!-- Use wave 10.0 to suppress warnings introduced in wave 11.0 --> | ||
| <!-- All existing warnings from wave 10.0 are still reported --> | ||
| <RazorWarningWave>10.0</RazorWarningWave> |
There was a problem hiding this comment.
I think RazorWarningLevel to match Roslyn (WarningLevel)
|
|
||
| 1. Should there be a way to opt-in to future warnings (set wave higher than language version) for early adopters? | ||
|
|
||
| 2. What is the minimum warning wave version we should support? Are values like 8 valid, or only 10+? |
There was a problem hiding this comment.
If we don't allow this, presumably that means someone targetting .NET 8 with the latest SDK just immediately gets errors about their warning wave, which they didn't even know about, right? Feels bad man.
4 participants
Add proposal for Warning Waves
Rendered document at https://github.com/chsienki/razor-tooling/blob/cebbe58b8b1387e792c932636f59143f80652952/docs/WarningWavesProposal.md