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

Don't enforce using key? and value? instead of has_key? and has_value? #161

Closed
matssigge opened this issue Mar 20, 2020 · 8 comments
Closed
Labels
rule change 👩‍⚖️ Suggest a style guide rule change

Comments

@matssigge
Copy link

matssigge commented Mar 20, 2020

In Hash, key?/has_key?and value?/has_value? are aliases of each other. To me, has_key? and has_value? are much clearer semantically, but StandardRB enforces using key? and value?. This is the default in RuboCop, which seems to stem from the fact that Matz prefers the short versions, and at some point even said the long versions were deprecated, although this was walked back. There is a discussion around this in rubocop/rubocop#3428, with this comment being especially relevant.

I would argue that has_key?and has_value? increase readability, and so we should be allowed to use them. Originally, I suggested that the preference be changed from enforcing the short versions to the long versions. But after @searls's comment below, I realize that it would be better just to remove this rule altogether, so that people can use the alias they prefer. So my proposal is exactly that - to disable the rule Style/PreferredHashMethods.

This might be a lost cause at this stage, but I would argue that has_key?and has_value? increase readability, and therefore make much more sense as the standard than the short versions. I propose changing the rubocop configuration accordingly.

I would be happy to provide a pull request, but since I'm guessing I'll be shot down, I'll wait until there's a verdict here. 😄

@searls searls added the rule change 👩‍⚖️ Suggest a style guide rule change label Mar 23, 2020
@searls
Copy link
Contributor

searls commented Mar 23, 2020

I've tripped over this one a couple times myself. I'll admit I don't feel very strongly about it. On one hand, I always like being terse. On the other hand, we've made a bunch of decisions in other cases not to enforce a particular alias be used (as it seems kind of heavy-handed to tell people what API they're allowed to call for a style tool, if there's no technical risk that'd warrant linting)

Will leave this open to see if anyone has a vote to share

@matssigge
Copy link
Author

As soon as I read your reply, I realized that the "correct" choice is probably either to leave it as is or to not enforce this at all. Changing from enforcing the short versions to enforcing the long ones will just introduce noise to the tool and force people into an unnecessary code change. So I hereby change my proposal to just removing this rule, i.e. not enforcing which methods to use in this case.

I'll update the title and description to reflect this.

@matssigge matssigge changed the title Prefer has_key? and has_value? over key? and value? Don't enforce using key? and value? instead of has_key? and has_value? Mar 23, 2020
@scottwater
Copy link

To me, Ruby's support of ? makes has obsolete.

I also think a simple ? is more consistent with other libraries like ActiveRecord.

@JonRowe
Copy link

JonRowe commented Mar 23, 2020

I'd vote for has_key? if I had to choose, but vote for not enforcing it except that it be consistent.

@matssigge
Copy link
Author

To me some_object.key?is asking "Hey, some_object, are you a key?" rather than "...do you have the key x?". You could of course argue that the parameter changes the meaning, but that requires more attention. I think has_key? is more straighforward in conveying the correct meaning in this particular case.

@soulcutter
Copy link

I vote for removing the rule and letting people use the API that makes sense for them.

@timriley
Copy link
Contributor

Long time follower, recent adoptee – I’d vote for disabling the rule entirely.

@searls searls closed this as completed in 67a1911 Mar 24, 2020
@searls
Copy link
Contributor

searls commented Mar 24, 2020

Agree with you all. Removed and landed in 0.2.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule change 👩‍⚖️ Suggest a style guide rule change
Projects
None yet
Development

No branches or pull requests

6 participants