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 #12309] Add new Style/SuperArguments cop #12427

Merged
merged 1 commit into from May 23, 2024

Conversation

Earlopain
Copy link
Contributor

@Earlopain Earlopain commented Dec 1, 2023

#12309 Implements the following:

def method(a, b, c:, d: &e)
  super(a, b, c:, d:, &e)
  # => super
end

There are 57 offenses of this kind in Rails master.

Looks like I picked quite the doozy for my first cop. I managed but I'm open to feedback on how to write better code here.
I tried to think of all the constellations you can use super in and handling those appropriately in the cop but I'm not certain if I got it all. Let me know if there are other edge-cases that need to be considered, like calling super inside define_singleton_method.

The cop could be more intelligent with keyword arguments when they appear in different orders like in the following example but I'll leave that be for now since the cop seems quite complex already. Something to improve upon in the future.

def test(a:, b:)
  super(b:, a:)
end
  • 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.

@koic
Copy link
Member

koic commented Dec 1, 2023

There might be users who intentionally write explicit arguments. So, I wonder if the cop name "Redundant" is suitable.
And, it would be beneficial to first suggest a rationale in the style guide for using zero arity super (zsuper) when super can have its arguments omitted:
https://github.com/rubocop/ruby-style-guide

@koic
Copy link
Member

koic commented Dec 1, 2023

It's likely that cop name such as Style/ZeroAritySuper cop may be better appropriate, I think.

@bbatsov
Copy link
Collaborator

bbatsov commented May 22, 2024

@Earlopain ping :-)

@Earlopain Earlopain force-pushed the add-redundant-super-arguments-cop branch from 35b91fa to c2e596b Compare May 22, 2024 10:52
@Earlopain
Copy link
Contributor Author

Oh, I forgot I even did this.

I renamed the cop to the suggestion from koic and disabled it by default. I don't fell strongly about this anymore to discuss about this in the styleguide, so if disabled by default is not good then feel free to close this.

@bbatsov
Copy link
Collaborator

bbatsov commented May 22, 2024

I think the cop is useful, and I even thought we already had something like it.

As far as the names go - I'm not sure I like ZeroAritySuper, as I find it a bit vague. Perhaps it could just be named SuperArguments, which would also leave the door to enforce removing or providing any redundant arguments. Anyways, I don't feel strongly about the name, but I'll leave this open for a bit more in case someone proposes a name that is clearer.

@rubocop/rubocop-core Any opinions on this would be welcome!

@bbatsov
Copy link
Collaborator

bbatsov commented May 22, 2024

I renamed the cop to the suggestion from koic and disabled it by default. I don't fell strongly about this anymore to discuss about this in the styleguide, so if disabled by default is not good then feel free to close this.

I don't think there's much to discuss about such a guideline as I thought that most people omit the redundant args, so you can safely file a PR in the style guide. I'm also not sure this should be disabled by default - probably we can leave it to the users to decide if they'd fine it useful or not.

@koic
Copy link
Member

koic commented May 22, 2024

Style/SuperArguments seems like a better name because it is appropriately abstract. I also think this cop can start with the default Enabled: pending.

@Earlopain Earlopain force-pushed the add-redundant-super-arguments-cop branch from 1cacf7a to f7d93e5 Compare May 22, 2024 16:17
@Earlopain
Copy link
Contributor Author

Earlopain commented May 22, 2024

Alright, I've openend rubocop/ruby-style-guide#941, set it back to pending and renamed to SuperArguments. I also agree that name is better.

I stumbled upon an interesting interaction I wasn't really aware of with blocks, see the styleguide PR for what I'm talking about. I can't think of any changes this cop would need because of this.

@Earlopain Earlopain force-pushed the add-redundant-super-arguments-cop branch from f7d93e5 to 4fabb89 Compare May 22, 2024 19:20
@Earlopain
Copy link
Contributor Author

Thanks for your review, I appreciate it. I should have addresses all your points

@Earlopain Earlopain force-pushed the add-redundant-super-arguments-cop branch from 4fabb89 to f99645f Compare May 23, 2024 06:26
@bbatsov bbatsov merged commit 3227bf0 into rubocop:master May 23, 2024
34 checks passed
@bbatsov
Copy link
Collaborator

bbatsov commented May 23, 2024

Thanks!

@Earlopain Earlopain deleted the add-redundant-super-arguments-cop branch May 23, 2024 10:03
@Earlopain Earlopain changed the title [Fix #12309] Add new Style/RedundantSuperArguments cop [Fix #12309] Add new Style/SuperArguments cop May 27, 2024
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