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

Mark parameter modifiers as deprecated and update spec #1693

Merged
merged 8 commits into from
Mar 6, 2021

Conversation

shenglol
Copy link
Contributor

@shenglol shenglol commented Mar 3, 2021

Contributing a Pull Request

Contributing a feature

  • I have opened a new issue for the proposal, or commented on an existing one, and ensured that the Bicep maintainers are good with the design of the feature being implemented
  • I have included "Fixes #{issue_number}" in the PR description, so GitHub can link to the issue and close it when the PR is merged
  • I have appropriate test coverage of my new feature

Closes #1670.
Closes #1727.

{
null => null,
DiagnosticLabel.Unnecessary => new Container<DiagnosticTag>(DiagnosticTag.Unnecessary),
DiagnosticLabel.Deprecated => new Container<DiagnosticTag>(DiagnosticTag.Deprecated),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how DiagnosticTag.Deprecated is rendered in VSCode:
image

@alex-frankel
Copy link
Collaborator

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.

@shenglol
Copy link
Contributor Author

shenglol commented Mar 3, 2021

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 @secure() only when the value of the secret property in the modifier gets resolved to true). I think it would take a few days. I'm open to send a follow-up PR to implement it if we think it's worth the effort.

Regarding the strikethrough thing, there's actually a setting to turn that off ("editor.showDeprecated": false). I know some people who like it and asked the VSCode team to add the feature, so personally I think we should keep it.

@alex-frankel
Copy link
Collaborator

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"?

@shenglol
Copy link
Contributor Author

shenglol commented Mar 3, 2021

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).

@shenglol shenglol changed the title Mark parameter modifiers as deprecated Mark parameter modifiers as deprecated and update spec Mar 4, 2021
@codecov-io
Copy link

codecov-io commented Mar 5, 2021

Codecov Report

Merging #1693 (b719712) into main (04999a1) will increase coverage by 0.01%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            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     
Flag Coverage Δ
dotnet 95.52% <91.66%> (+0.01%) ⬆️
typescript 26.61% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Bicep.Core/Diagnostics/FixableDiagnostic.cs 58.33% <50.00%> (ø)
...c/Bicep.Core/Diagnostics/FixableErrorDiagnostic.cs 58.33% <50.00%> (ø)
...icep.LangServer/Extensions/DiagnosticExtensions.cs 86.36% <77.77%> (+0.64%) ⬆️
src/Bicep.Cli.IntegrationTests/ProgramTests.cs 100.00% <100.00%> (ø)
...Core.IntegrationTests/Emit/TemplateEmitterTests.cs 100.00% <100.00%> (ø)
...Tests/Assertions/DiagnosticCollectionExtensions.cs 87.50% <100.00%> (+1.78%) ⬆️
src/Bicep.Core/Diagnostics/Diagnostic.cs 100.00% <100.00%> (ø)
src/Bicep.Core/Diagnostics/DiagnosticBuilder.cs 100.00% <100.00%> (ø)
src/Bicep.Core/Diagnostics/ErrorDiagnostic.cs 66.66% <100.00%> (ø)
src/Bicep.Core/Semantics/SymbolValidator.cs 84.21% <100.00%> (ø)
... and 5 more

@majastrz
Copy link
Member

majastrz commented Mar 5, 2021

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.

@majastrz
Copy link
Member

majastrz commented Mar 5, 2021

Btw, I think the phases for modifier deprecation should be the following:
0.4 - block modifier usage with errors, parser/semantic model still understands modifiers (the latter is needed to make the code fix reliable)
0.5 - parser/semantic model no longer understands modifiers (we would also remove the code fix)

@shenglol
Copy link
Contributor Author

shenglol commented Mar 5, 2021

Makes sense. I'll remove the strikethrough. The deprecation plan also sounds good to me.

@majastrz
Copy link
Member

majastrz commented Mar 5, 2021

No, I'm saying that it's OK that it's aggressive.

@majastrz
Copy link
Member

majastrz commented Mar 5, 2021

Also interested in what others think about it.

@shenglol
Copy link
Contributor Author

shenglol commented Mar 5, 2021

No, I'm saying that it's OK that it's aggressive.

Ah misread your message...I think we should have a DSL discussion on it.

@majastrz
Copy link
Member

majastrz commented Mar 5, 2021

Makes sense.

@alex-frankel
Copy link
Collaborator

alex-frankel commented Mar 5, 2021

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?

@majastrz
Copy link
Member

majastrz commented Mar 5, 2021

Good pt.

docs/spec/parameters.md Outdated Show resolved Hide resolved
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:
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow 2M+ downloads! Just installed it too!


In reply to: 588794961 [](ancestors = 588794961)

@@ -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();
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

@majastrz majastrz left a 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.

@shenglol shenglol enabled auto-merge (squash) March 6, 2021 00:54
@shenglol shenglol requested a review from majastrz March 6, 2021 00:54
Copy link
Member

@majastrz majastrz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@shenglol shenglol merged commit af62b28 into main Mar 6, 2021
@shenglol shenglol deleted the shenglol/deprecate-param-modifiers branch March 6, 2021 02:53
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.

Add documentation for decorators Mark parameter modifiers as deprecated
4 participants