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

Replace UnusedVariable with UnusedParameter for parameters #195

Merged
merged 2 commits into from
Sep 9, 2020

Conversation

sirbrillig
Copy link
Owner

@sirbrillig sirbrillig commented Jul 28, 2020

This (breaking!) change replaces UnusedVariable warnings for function arguments with UnusedParameter and UnusedParameterBeforeUsed.

The options allowUnusedParametersBeforeUsed and allowUnusedFunctionParameters continue to work as before including that allowUnusedParametersBeforeUsed defaults to true, meaning that UnusedParameterBeforeUsed will never show normally unless that option is explicitly set to false.

Fixes #175

@sirbrillig sirbrillig marked this pull request as draft July 28, 2020 16:40
@sirbrillig sirbrillig self-assigned this Jul 31, 2020
@sirbrillig
Copy link
Owner Author

I don't feel super-comfortable with this because the current default behavior is to hide warnings for unused parameters before used ones and this means that in creating a new sniff code for those warnings, we have to either:

  • Do nothing and let the new warning be disabled manually by folks who want that behavior. This inverts the default and is IMO a poor experience for developers since the original reason that option was made the default was to be less noisy for a common use-case.
  • Do nothing but set the severity of the new warning to 0 in the built-in ruleset.xml. This seems to be the best compromise (and it's what I've done above) but it feels like a bit of a hack and I don't know of any other sniff that has done something like this before. It also isn't quite the same because the sniff itself still reports those warnings (this is why many of the tests are failing in this branch currently). The other downside is that in order to deprecate and not remove the option itself, we need to invert its default, which makes it a little difficult to report the deprecation since it's hard to know if the option has been changed from its original default.

Anecdotally, looking at the similar no-unused-vars rule in eslint, they have only the one rule with an option to ignore unused before used like we already have, and it is also enabled by default like we already have.

As this is delaying v3.0 I'm inclined to drop this from the milestone and move it to 4.0 instead to give us some time to figure out what best to do here. As this was originally your feature suggestion @jrfnl, I'd love to hear if you have any thoughts on the matter.

@sirbrillig sirbrillig added this to the 3.0 milestone Aug 10, 2020
@sirbrillig sirbrillig modified the milestones: 3.0, 4.0 Aug 18, 2020
@sirbrillig sirbrillig force-pushed the add-unused-parameter-warnings branch from abb7d1d to 5064a9c Compare September 9, 2020 23:54
@sirbrillig sirbrillig marked this pull request as ready for review September 9, 2020 23:55
@sirbrillig
Copy link
Owner Author

I modified this PR so that it no longer deprecates the old options. That way allowUnusedParametersBeforeUsed will continue to work as before. We can consider deprecating the options in a later PR.

@sirbrillig sirbrillig merged commit 1adb848 into master Sep 9, 2020
@sirbrillig sirbrillig deleted the add-unused-parameter-warnings branch September 9, 2020 23:56
@sirbrillig sirbrillig modified the milestones: 4.0, 3.0 Sep 9, 2020
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.

Improve errorcode differentiation
1 participant