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

Add new Style/OptionalBooleanParameter cop #8412

Merged
merged 1 commit into from Jul 30, 2020

Conversation

fatkodima
Copy link
Contributor

https://rubystyle.guide/#boolean-keyword-arguments

# bad
def some_method(bar = false)
  puts bar
end

# good
def some_method(bar: false)
  puts bar
end

Style/BooleanKeywordArgument:
Description: 'Use keyword arguments when defining method with boolean argument.'
StyleGuide: '#boolean-keyword-arguments'
Enabled: pending
Copy link
Member

Choose a reason for hiding this comment

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

I think that this cop is unsafe, just like Style/OptionalArguments cop.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious why Style/OptionalArguments is "unsafe", or this one?

For this one, my only concern is if there is a call to super, or super(that_argument), then maybe there should be no offense since this method is just copying the interface it inherits. Maybe there should not be an offense then; the base method will be flagged probably. What other cases are unsafe?

Copy link
Member

Choose a reason for hiding this comment

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

I think it means a breaking change to users if the API signature is a published interface.
https://martinfowler.com/bliki/PublishedInterface.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it unsafe.

Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

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

I love it when a new cop improves our codebase!

Here, I think all of these true/false were clear examples of arguments that can not be understood when called.

lib/rubocop/cop/registry.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/style/boolean_keyword_argument.rb Outdated Show resolved Hide resolved
@fatkodima fatkodima force-pushed the boolean_keyword_argument-cop branch from 12b12f2 to 786d100 Compare July 29, 2020 17:30
@bbatsov
Copy link
Collaborator

bbatsov commented Jul 30, 2020

Seems we've made a mistake with the heading in the style guide - I think the cop should be named OptionalBooleanParameter.

@fatkodima fatkodima force-pushed the boolean_keyword_argument-cop branch from 786d100 to cd8d521 Compare July 30, 2020 08:29
@fatkodima fatkodima changed the title Add new Style/BooleanKeywordArgument cop Add new Style/OptionalBooleanParameter cop Jul 30, 2020
@fatkodima fatkodima force-pushed the boolean_keyword_argument-cop branch from cd8d521 to 45250d2 Compare July 30, 2020 08:35
@fatkodima
Copy link
Contributor Author

Updated with new name.

@bbatsov bbatsov merged commit df70484 into rubocop:master Jul 30, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Jul 30, 2020

Great cop!

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