-
Notifications
You must be signed in to change notification settings - Fork 759
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
Mark parameter modifiers as deprecated and update spec #1693
Conversation
{ | ||
null => null, | ||
DiagnosticLabel.Unnecessary => new Container<DiagnosticTag>(DiagnosticTag.Unnecessary), | ||
DiagnosticLabel.Deprecated => new Container<DiagnosticTag>(DiagnosticTag.Deprecated), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much more work would the quick fix be to switch to the decorator syntax? I wonder if we should show just a regular warning instead of a strikethrough to start. |
It's not something very hard to do, but it's also not that straightforward as there are quite a few edge cases to handle (e.g. make sure to generate Regarding the strikethrough thing, there's actually a setting to turn that off ( |
Makes sense re: quick fix, so let's decide separately what we want to do there. WRT the strikethrough, it just feels a little aggressive to me, but I'm curious what others think. Can we also include a link in the warning message so people understand what we mean when we way "use decorators"? |
I see. The the VSCode feature request microsoft/vscode#50972 received a number of 👍s which makes me think we should strikeout modifiers, but I'm totally fine removing it if that's a bit aggressive. I would also like to hear @anthony-c-martin , @majastrz and @TarunSunkaraneni 's thoughts on this. Including a link to the parameter spec in the warning message is a good idea and I'll do it (just realized that the examples in the spec are still using modifiers so I'll update them in the PR, too). |
Codecov Report
@@ Coverage Diff @@
## main #1693 +/- ##
==========================================
+ Coverage 95.07% 95.08% +0.01%
==========================================
Files 370 370
Lines 21446 21477 +31
Branches 15 15
==========================================
+ Hits 20389 20421 +32
+ Misses 1057 1056 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
I do see the point that the default strike-through style is a bit aggressive. We are saying "this is going to break in the future" vs "this is a potential problem", so I feel like the distinction makes it justifiable. Implementing the code fix makes the experience better also because then there's an instant mechanism to fix it which removes the strike-through styling. |
Btw, I think the phases for modifier deprecation should be the following: |
Makes sense. I'll remove the strikethrough. The deprecation plan also sounds good to me. |
No, I'm saying that it's OK that it's aggressive. |
Also interested in what others think about it. |
Ah misread your message...I think we should have a DSL discussion on it. |
Makes sense. |
I wouldn't let this open question block check-in though. Since it's just a flag, we can play with it and make a final decision before the next release? |
Good pt. |
docs/spec/parameters.md
Outdated
A parameter cannot have the same name as a [variable](./variables.md), [resource](./resources.md), [output](./outputs.md) or another parameter in the same scope. | ||
|
||
## Declration with decorators | ||
Decorators provide a way to attach constrains and metadata to a parameter. Decorators are placed above the paramter declaration to be decorated. They use the form @expression, where expression must be a function call: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
paramter [](start = 107, length = 8)
typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. I just found this VSCode extension and installed it. Not surprisingly it discovered a bunch more typos in the code and docs...I will send a separate PR to fix all of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -104,7 +104,8 @@ public void ValidBicepTextWriter_TemplateEmiterTemplateHashCheck(DataSet dataSet | |||
|
|||
// emitting the template should be successful | |||
var result = this.EmitTemplate(SyntaxTreeGroupingBuilder.Build(new FileResolver(), new Workspace(), PathHelper.FilePathToFileUrl(bicepFilePath)), memoryStream, ThisAssembly.AssemblyFileVersion); | |||
result.Diagnostics.Should().BeEmpty(); | |||
// TODO: remove Where when the the support of parameter modifiers is dropped. | |||
result.Diagnostics.Where(d => d.Code != "BCP153").Should().BeEmpty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where [](start = 31, length = 5)
I wonder if there's any value in creating a helper class (ParameterModifierDeprecationHelper
?) that contains all the special test cases for this migration. Later on, we can just delete the class and follow the compiler errors to clean up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah makes. I found that there's already the DiagnosticCollectionExtensions
class so I added an extension method to it, which might be simpler than adding another helper class.
src/Bicep.Core.Samples/Files/Parameters_LF/main.diagnostics.bicep
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found some doc typos and added some optional suggestions in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Contributing a Pull Request
Contributing a feature
Closes #1670.
Closes #1727.