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

[Fix #8506] Add AllowedParentClasses config to Lint/MissingSuper. #11940

Conversation

iMacTia
Copy link
Contributor

@iMacTia iMacTia commented Jun 8, 2023

Add AllowedParentClasses config to Lint/MissingSuper.

The default value of %w[BasicObject Object] is kept for backwards compatibility, but users can now add to that list using the AllowedParentClasses config value.

It's important to note that the value of AllowedParentClasses will be ADDED to the default value.
This is because I don't foresee scenarios where users would want to remove BasicObject or Object from this list, and removes the requirement for always specifying them.
I'm open to feedback on this assumption and happy to change it into a full replacement, I simply went with the most logical approach in my opinion.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@iMacTia iMacTia force-pushed the fix/lint-missing-super-configurable-stateless-classes branch from 313e068 to c940c51 Compare June 8, 2023 13:26
@Drenmi
Copy link
Collaborator

Drenmi commented Jun 9, 2023

Change looks good, but perhaps we can generalize the setting name to divorce it from one particular reason for ignoring a node. E.g. ExcludedParentClasses or similar? 🙂

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 12, 2023

Agreed - the name should be more generic. Also this settings needs to be documented and some example usages should be added to the docs.

@iMacTia
Copy link
Contributor Author

iMacTia commented Jun 12, 2023

Change looks good, but perhaps we can generalize the setting name

That makes sense, ExcludedParentClasses conveys the intent rather than the reason, I'll change it to that 👍

Also this settings needs to be documented and some example usages should be added to the docs

I wasn't sure about this, I'll add something. Thanks for pointing it out

@iMacTia iMacTia force-pushed the fix/lint-missing-super-configurable-stateless-classes branch from c940c51 to 77a425b Compare June 12, 2023 09:06
@iMacTia iMacTia changed the title [Fix #8506] Add StatelessClasses config to Lint/MissingSuper. [Fix #8506] Add ExcludedParentClasses config to Lint/MissingSuper. Jun 12, 2023
@iMacTia iMacTia force-pushed the fix/lint-missing-super-configurable-stateless-classes branch from 77a425b to 91d5809 Compare June 12, 2023 12:42
@iMacTia
Copy link
Contributor Author

iMacTia commented Jun 12, 2023

Updated and force-pushed to keep the commit history clean.
@Drenmi @bbatsov this should now be ready for a second pass 👍

classes from this cop, for example in the case of an abstract class that is
not meant to be called with `super`. In those cases, you can use the
`ExcludedParentClasses` option to specify which classes should be excluced
*in addition to* `Object` and `BasicObject`.
Copy link
Member

Choose a reason for hiding this comment

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

This document and examples are auto-generated based on the source code. Please update lib/rubocop/cop/lint/missing_super.rb directly.
There's no need to update the docs/modules/ROOT/pages/cops_lint.adoc doc as it will be generated at the time of release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @koic, I've amended the commit to keep the history clean.
I hope I did it right 😄

@iMacTia iMacTia force-pushed the fix/lint-missing-super-configurable-stateless-classes branch from 91d5809 to 84f2c69 Compare June 12, 2023 16:41
Copy link
Member

@koic koic left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@@ -60,6 +67,22 @@ module Lint
# end
# end
#
# @example ExcludedParentClasses: [MyAbstractClass]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be named AllowedParentClasses to be in-line with how we typically name stuff. /cc @koic

Copy link
Member

Choose a reason for hiding this comment

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

I agree the new suggested name :-)

@@ -14,6 +14,13 @@ module Lint
# Autocorrection is not supported because the position of `super` cannot be
# determined automatically.
#
# `Object` and `BasicObject` are excluded from this cop because of their
Copy link
Collaborator

Choose a reason for hiding this comment

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

excluded from -> allowed by

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 19, 2023

Other than my remark about the naming the changes look good as well. I'll merge the PR once the name is updated.

@iMacTia iMacTia force-pushed the fix/lint-missing-super-configurable-stateless-classes branch from 84f2c69 to c6c842d Compare June 20, 2023 10:32
@iMacTia iMacTia changed the title [Fix #8506] Add ExcludedParentClasses config to Lint/MissingSuper. [Fix #8506] Add AllowedParentClasses config to Lint/MissingSuper. Jun 20, 2023
@iMacTia
Copy link
Contributor Author

iMacTia commented Jun 20, 2023

Thank you @bbatsov, the new suggested name makes sense.
I've updated the branch (amended to keep commit history clean), as well as the PR title and description

# # there's no need to specify Object or BasicObject
# # in the AllowedParentClasses option.
# end
# end
Copy link
Member

Choose a reason for hiding this comment

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

The example ClassWithNoParent class is irrelevant to the AllowedParentClasses configuration and is always allowed, so it looks better to put it in the # good case of @example, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, I can move it there, but I wanted to make it absolutely clear that no matter what the value of AllowedParentClasses is, Object and BasicObject will always be allowed, so this example is just to remark that one more time in case the reader misses it from the description above. Although it can technically go in both places, I feel like it's more useful here. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I understand the intention. But @example ConfigurationName is typically used to describe unique behaviors of ConfigurationName. For the sake of consistent documentation structure, it would be better to move this to @example I think :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't really argue with that, I understand the value of consistency 🙂
I'll move this up as suggested 👍

The default value of %w[BasicObject Object] is kept for backwards compatibility, but users can now add to that list using the AllowedParentClasses config value.
@iMacTia iMacTia force-pushed the fix/lint-missing-super-configurable-stateless-classes branch from c6c842d to 09d0ff5 Compare June 21, 2023 08:26
@iMacTia
Copy link
Contributor Author

iMacTia commented Jun 21, 2023

I've got a "rubocop server is not running" error while running specs on 2.7, but that might just be a flaky?
I can't re-run the test though, so I'll need your help 🙏

@koic
Copy link
Member

koic commented Jun 21, 2023

Yeah, I have rerun the CI matrix contains flaky spec. Thank you.

@koic koic merged commit 1e2d84a into rubocop:master Jun 21, 2023
29 checks passed
@iMacTia iMacTia deleted the fix/lint-missing-super-configurable-stateless-classes branch June 21, 2023 16:30
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

4 participants