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

Provide remove unused parameter/variable as quickfix. Fixes #5613 #5804

Merged

Conversation

nilshedstrom
Copy link
Contributor

This PR adds quickfixes for the linter rules no-unused-vars and no-unused-vars as suggested in issue #5613.

Because of failing unit tests I decided not provide linter rules no-unused-vars and no-unused-params for invalid parameters/variables (missing names)

@StephenWeatherford @majastrz

@anthony-c-martin
Copy link
Member

I think there are some issues in how multiline statements and statements with leading/trailing comments are handled. Here are some edge cases that repro issues and would be good to add as test cases:

var adsf = 'asdf' /* 
adf 
*/
var asdf = {
  abc: 'def'
}
var abcd = concat('foo',/*
*/'bar')
var multiline = '''
This
is
a
multiline
'''
@description('''
''')
var asdf = 'asdf'
var foo = 'asdf' // asdef
/* asdfds */ var foo = 'asdf'
#disable-next-line foo
param string asdf = 123
/* asdf */ var foo = 'asdf'
var bar = 'asdf'

@nilshedstrom
Copy link
Contributor Author

@StephenWeatherford can you clearify what kind of response you are waiting for (what am I supposed to fix)
I did fix the comment from @anthony-c-martin

@StephenWeatherford
Copy link
Contributor

Thanks. Looked like there were still unresolved comments. GitHub's CR process is very lacking. I'll take a look. Always feel free to nudge if it looks like something is getting forgotten.

@StephenWeatherford
Copy link
Contributor

Sorry for the delay, haven't forgotten.

@StephenWeatherford StephenWeatherford self-assigned this May 4, 2022
Copy link
Contributor

@StephenWeatherford StephenWeatherford left a comment

Choose a reason for hiding this comment

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

A few minor changes requested

@nilshedstrom
Copy link
Contributor Author

A few minor changes requested

I did the changes you requested.

Copy link
Contributor

@StephenWeatherford StephenWeatherford left a comment

Choose a reason for hiding this comment

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

Ideally, "type" would be an enum, not a string.

I'll leave it up to you if you want to update. Thanks for adding this!

@StephenWeatherford
Copy link
Contributor

Re-running tests.

@StephenWeatherford
Copy link
Contributor

Looks like some of the "invalid variable" filter has regressed in the test baseline:

image

@StephenWeatherford
Copy link
Contributor

@nilshedstrom I'll be OOF starting tomorrow for a couple of weeks. If I don't respond, @bhsubra can help finish the review.

@StephenWeatherford
Copy link
Contributor

Hm, looks like we're getting a null ref exception during the tests in an unrelated rule:

Expected AssertionExtensions {"[linter-internal-error (Warning)] Analyzer 'core/no-hardcoded-location' encountered an unexpected exception. Object reference not set to an instance of an object.", "[BCP152 (Error)] Function "resourceId" cannot be used as a decorator.", "[no-unused-params (Warning)] Parameter "foo" is declared but never used.", "[BCP152 (Error)] Function "concat" cannot be used as a decorator.", "[no-unused-vars (Warning)] Variable "bar" is declared but never used.", "[BCP152 (Error)] Function "environment" cannot be used as a decorator.", "[BCP081 (Warning)] Resource type "Microsoft.Network/virtualNetworks@2020-06-01" does not have types available.", "[BCP152 (Error)] Function "union" cannot be used as a decorator.", "[BCP152 (Error)] Function "guid" cannot be used as a decorator."} to not have any items matching (x.Code == "linter-internal-error") because Should never get LinterAnalyzer.LinterRuleInternalError, but found {"[linter-internal-error (Warning)] Analyzer 'core/no-hardcoded-location' encountered an unexpected exception. Object reference not set to an instance of an object."}.

Re-running test. Never seen that before.

Copy link
Contributor

@StephenWeatherford StephenWeatherford left a comment

Choose a reason for hiding this comment

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

A couple of suggested but not required changes. We just need to make sure the tests pass.

@StephenWeatherford
Copy link
Contributor

Manually fixed merge conflicts

@StephenWeatherford
Copy link
Contributor

@nilshedstrom Thanks! Looks great!

@StephenWeatherford StephenWeatherford merged commit b8393fa into Azure:main May 31, 2022
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.

None yet

3 participants