search

jeremyevans / sequel

Sequel: The Database Toolkit for Ruby
http://sequel.jeremyevans.net
Other
4.98k stars 1.07k forks source link

alias key? has_key? #1998

Closed TSMMark closed 1 year ago

TSMMark commented 1 year ago

Hash#key? and Hash#has_key? are aliases of each other in Ruby. This would bring pg_json_ops in line with similar aliases.

Additionally, Rubocop has Style/PreferredHashMethods which prefers key? over has_key? by default. Of course rubocop does not know which objects are Hashes and which are pg json op, it registers offenses when using has_key? on pg json op objects.

This would be a small QOL improvement for users of rubocop, and all accustomed to key? over has_key?

jeremyevans commented 1 year ago

If we were going for consistency with Hash, we would also have to add member? as another alias.

Personally, I have a strong feelings against adding code to appease pointless, unwise, and incorrect linting requirements. Pointless because it doesn't matter which method you use. Unwise because without context key? as a method name should be asking whether the receiver is a key, not whether it has a key, so has_key? is a better method name if you had to pick between the two. Incorrect because this isn't a hash and therefore Hash-specific linting does not apply.

I'm generally against adding aliases at all unless an existing method name is misleading or in some way problematic (most aliases in Sequel already existed when I took over maintenance), and this method already has an alias. Implementation-wise, I'm also against patches that add features without appropriate tests. :)

All that being said, I'll consider this if there is broad community support for it. So I'll leave this open for a while to collect feedback from the community.

benkoshy commented 1 year ago

All that being said, I'll consider this if there is broad community support for it. So I'll leave this open for a while to collect feedback from the community.

I appreciate the PR - I thank you for providing it. I also appreciate the maintainer's view - which FWIW, I concur with.

(I use sequel in a hobby context - small personal protects - so perhaps weigh this opinion accordingly).

jeremyevans commented 1 year ago

I'm closing this now. It's easy to add the alias itself if you need it.

tycooon commented 1 year ago

Actually there is a bunch of issues that people might have when using Rubocop with Sequel. For example, we have the following Rubocop configs in one of our projects:

Style/NumericPredicate:
  Enabled: false # Doesn't work with sequel

Style/PreferredHashMethods:
  Enabled: false # Doesn't work with sequel

Style/ConcatArrayLiterals:
  Enabled: false # Doesn't work with sequel

So maybe at some point someone will come up with a plugin that adds a bunch of "missing" aliases like the one in this PR.

jeremyevans commented 1 year ago

So maybe at some point someone will come up with a plugin that adds a bunch of "missing" aliases like the one in this PR.

I'd be happy to add a link on Sequel's website to an external extension that did that.