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

Rename {super,sub} set methods to clearly indicate relation of operands #10020

Closed
straight-shoota opened this issue Dec 3, 2020 · 3 comments · Fixed by #10187
Closed

Rename {super,sub} set methods to clearly indicate relation of operands #10020

straight-shoota opened this issue Dec 3, 2020 · 3 comments · Fixed by #10187

Comments

@straight-shoota
Copy link
Member

I just realized that methods like Set#subset? apply in exactly the opposite direction to what I might expect.
I would probably read a predicate method subset?(b) as "is b a subset? (of the receiver)", i.e. b ⊂ a But instead a.subset?(b) returns true if a ⊂ b.

These semantics are the same as in Ruby. I suspect they have it that way to have the same operand relation as with the comparison operators. But it's really confusing. When reading the code, you can't beyond doubt recognize which way it is.

So I'm proposing to rename these methods to a differnt, less ambiguous name. Adding a simple _of suffix would fix this. a.subset_of?(b) clearly means a ⊂ b.

  • Set#subset? => Set#subset_of?
  • Set#proper_subset? => Set#Proper_subset_of?
  • Set#superset? => Set#superset_of?
  • Set#proper_superset? => Set#proper_superset_of?

This also affects proposed similar methods for Hash (see #7500 (comment))

@asterite
Copy link
Member

Bang methods apply to the receiver. a.includes?(b) means "does a include b"? So a.subset?(b) naturally means "is a a subset of b?". So I don't this needs to change.

That said, adding the "of" suffix makes it a bit more readable.

@straight-shoota
Copy link
Member Author

I don't follow how you see a relation to bang methods. This is not about question mark representing nilable methods. Even if there is one, it's not obvious, which supports the argument that the API should be more explicit.

These are all predicate methods. And as such, either way to read the relation between receiver and arg is possible. And as I mentioned above, I intuitively read a.subset?(b) as asking a: "is b a subset?"
Removing this ambiguity is a good improvement.

@asterite
Copy link
Member

Oh, sorry, I meant "question method".

And as I mentioned above, I intuitively read a.subset?(b) as asking a: "is b a subset?"

That's the thing. Whenever there's a.method?(b) it means "Is 'a method? b'" true? In the case of a.subset?(b) it means "is a a subset of b"? In the case of a.includes?(b)" is "does 'a includes b' hold true?". aalways appears **before**b` in the sentence.

A question method is always asking "Are you ...?" or "Do you ...?". It never asks something about the argument.

That said, I think the "of" suffix makes this a bit clearer.

Sija added a commit to Sija/crystal that referenced this issue Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants