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

Allow non_bare_access_modifier_declaration? to handle modifiers with multiple arguments #325

Merged
merged 1 commit into from
Nov 2, 2024

Conversation

dvandersluis
Copy link
Member

I noticed this when testing Style/AccessModifierDeclarations. When an access modifier has multiple arguments, non_bare_access_modifier_declaration? erroneously (IMO) returns false, which causes the cop to not recognize all cases it should.

The cop does not properly recognize (as a positive or negative, depends on the AllowModifiersOnSymbols configuration) this despite the examples for the cop including it.

private :foo, :bar

I have a follow up PR to Style/AccessModifierDeclarations coming once this can be merged.

@dvandersluis
Copy link
Member Author

Hmm there's a Style/AccessModifierDeclarations failure in internal_investigation now because of this change but it's a catch 22 since it needs the cop to be updated which needs AST to be updated... 😅

@dvandersluis
Copy link
Member Author

I added that offense to .rubocop_todo.yml for now. I'll clean it up after everything is fixed.

(I didn't want to regenerate the whole file because it changes significantly and adds offenses to other files to remove # rubocop:disable directives -- we should maybe do this later though?)

@marcandre marcandre merged commit 2515451 into rubocop:master Nov 2, 2024
19 checks passed
@marcandre
Copy link
Contributor

Thanks for fixing this bug.

@dvandersluis dvandersluis deleted the access-modifier branch November 2, 2024 18:29
@marcandre
Copy link
Contributor

Released 1.33.1

@dvandersluis
Copy link
Member Author

Thanks @marcandre! ❤️

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.

2 participants