-
Notifications
You must be signed in to change notification settings - Fork 742
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
Apply SimplifyInterpolationRule to all expressions #6152
Apply SimplifyInterpolationRule to all expressions #6152
Conversation
Update the SimplifyInterpolationRule linter rule to apply to all expressions that resolve to strings, not just identifiers of string variables and parameters.
@jeskew There are some baseline test failures in. In the contribution guide, there are some instructions on how to force the tests to regenerate the baseline assets. Then, you just manually review the git diffs to ensure that the changes are legitimate. |
@jeskew Thanks! I should be able to take a look today or tomorrow. Would love to have this fixed. |
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.
Thanks for fixing!
Re-running suites (timeout issue?) |
@majastrz My original code had this comment indicating why it wasn't done:
It sounds like the rule should only trigger if the expression type is string? Does that sound right to you? |
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.
We need to resolve the comment I made to Marcin before merging
@StephenWeatherford The |
Ah, I missed that there was aleady a IsStrictlyAssignableToString(type)) check. Thanks! |
src/Bicep.Core/Analyzers/Linter/Rules/SimplifyInterpolationRule.cs
Outdated
Show resolved
Hide resolved
…e.cs Co-authored-by: Stephen Weatherford (MSFT) <[email protected]>
Trying build again - I've had some issues with it recently. |
@StephenWeatherford The failing tests are reporting errors about being unable to locate credentials for live tests. It looks like these tests require access to a couple repository secrets, so I believe those failures are expected given GitHub's policy of not sharing secrets with pull requests originating from forks. |
Live tests aren't marked as required. We'll be updating the CI configuration to exclude them in fork PRs to avoid confusion in the future. |
Fixes #5963
Update the SimplifyInterpolationRule linter rule to apply to all expressions that resolve to strings, not just identifiers of string variables and parameters.
Contributing a Pull Request
If you haven't already, read the full contribution guide. The guide may have changed since the last time you read it, so please double-check. Once you are done and ready to submit your PR, run through the relevant checklist below.
Contributing a feature