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

Style/OptionHash is possibly overeager #2106

Closed
wli opened this issue Aug 5, 2015 · 9 comments
Closed

Style/OptionHash is possibly overeager #2106

wli opened this issue Aug 5, 2015 · 9 comments

Comments

@wli
Copy link
Contributor

wli commented Aug 5, 2015

First, I know this is disabled by default, so it's not super critical. However, I'd love to enable this cop by default in my projects.

I have the following function signature:

def get_unique_photo(photo_size = :large, used_photos = {})

used_photos isn't an option hash - it actually stores data that we use to calculate certain properties. I could change this to keyword arguments to get around this cop (or locally disable), but it doesn't seem like the right solution.

What do you think of changing this so it only errors out for options = {}, args = {}, or some other whitelist of acceptable names?

@maxjacobson - this was originally added in #2030

@maxjacobson
Copy link
Contributor

hey @wli!

I did consider that...

What would the list be? I started thinking about people programming Ruby using non-English words for variable names and got dizzy

@maxjacobson
Copy link
Contributor

Curious to hear from @yob and @bbatsov on this

@wli
Copy link
Contributor Author

wli commented Aug 5, 2015

I can see how maintaining a list can be a nightmare. Maybe you can make an option on the cop for individual projects to specify their own whitelist/blacklist? Then rubocop-the-project doesn't care, but it'll work for more use cases.

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 6, 2015

I'm OK with having a list of param names that would trigger the offense.

@jfelchner
Copy link
Contributor

@wli I think that's a great idea. Maybe with some safe defaults: options and args makes sense. Maybe look at rails/rails to get some more ideas?

@jfelchner
Copy link
Contributor

Just throwing out more ideas for defaults: rest, params, parameters...

The default list should be something extremely safe that people aren't going to complain about and then, if they don't want it, they can change it to whatever they want.

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 6, 2015

Sounds reasonable.

On 6 August 2015 at 12:10, Jeff Felchner notifications@github.com wrote:

Just throwing out more ideas for defaults: rest, params, parameters...


Reply to this email directly or view it on GitHub
#2106 (comment).

Best Regards,
Bozhidar Batsov

http://www.batsov.com

@wli
Copy link
Contributor Author

wli commented Aug 6, 2015

I took a stab at this: #2114

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 22, 2015

Fixed by #2114

@bbatsov bbatsov closed this as completed Aug 22, 2015
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

4 participants