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

About the NotImplementedError exception for Lint/UnusedMethodArgument #11393

Open
sandstrom opened this issue Jan 5, 2023 · 0 comments
Open

Comments

@sandstrom
Copy link

sandstrom commented Jan 5, 2023

First of all, thanks for an awesome project! 🏅

The current implementation will not complain about unused method arguments if the method raises a NotImplementedError (it was implemented in this PR: #7739).

However, many recommend against raising this exception in your code:

These three posts all boil down to:

  1. Its actual meaning is different than the name implies – according to RubyDoc it should be raised when a feature is not implemented on the current platform, think of low-level things like calling fork or fsync.
  2. It's a descendant of ScriptError so a standard rescue block will not capture this, which may cause unexpected problems.

There is also ongoing discussion in this sibling repo to Rubocop:

Downside

It's somewhat weird to have def read_db(_name), where the real method name is name.

Possible solution

  • Add an option called IgnoredExceptions (array type), with the default value of ['NotImplementedError'].
    • For anyone who hasn't configured this cop, it's 100% backwards compatible.
      • People who have set IgnoreNotImplementedMethods to false would need to update their config to use IgnoredExceptions: [] intead.
    • Could then deprecate the current option called IgnoreNotImplementedMethods and remove it in the next major.
  • Allow the yarddoc @abstract keyword to be used as another signal to disable this lint check on a method.
  • Remove the exception case for NotImplementedError (if you think the reasons for doing so makes sense).
  • Do nothing and consider this an edge case.

Docs for Lint/UnusedMethodArgument

image

Addendum

Also, I realize this is nuanced. So feel free to close this issue if you think it's irrelevant or that the current logic is "good enough".

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

No branches or pull requests

1 participant