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 domain.member? #5041

Closed
mppf opened this issue Dec 15, 2016 · 9 comments
Closed

rename domain.member? #5041

mppf opened this issue Dec 15, 2016 · 9 comments

Comments

@mppf
Copy link
Member

mppf commented Dec 15, 2016

The current method name for querying if a value is in a domain is domain.member().

E.g.

var D:domain(int);

D += 4;

assert(D.member(4));
assert(!D.member(3));

This method name bothers me for English/Programming Style reasons. member is a noun and I really want the method name to be a verb in this case.

What should we do?

  1. add domain.contains()
    a) and deprecate domain.member()
    b) and keep domain.member() as a synonym
  2. enable parser/build to translate x in someDomain into this query (Support 'in' as an infix operator for membership test #5034)
    a) and deprecate domain.member()
    b) and keep domain.member() as a synonym
  3. do nothing
@mppf
Copy link
Member Author

mppf commented Dec 15, 2016

I created PR #5026 to add domain.contains(). I think we should add a deprecation warning to domain.member(), but leave it around (no-doc'd) for some time, since this is just a trivial renaming. In other words I'm in favor of 1a above.

@bradcray
Copy link
Member

If we add a deprecation warning, I think we should add a test case that triggers it in test/deprecated and then remove it after the next release. I.e., I don't think there's great value in carrying deprecated features across multiple releases and would prefer to avoid carrying too much deprecated code around. I could possibly imagine an argument to carry it across two releases in case professors only teach a class using Chapel once a year, particularly in this case due to the triviality of carrying the code forward.

@mppf
Copy link
Member Author

mppf commented Dec 16, 2016

@lydia-duncan, @bradcray, @ben-albrecht: OK, my current plan is to:

  • add domain.contains
  • deprecate domain.member() with a warning (for 1.15 and 1.16)
  • remove domain.member() in 1.17

Sound OK? If I understand correctly this is OK with each of you...

Thanks.

@lydia-duncan
Copy link
Member

Sounds good to me!

@ben-albrecht
Copy link
Member

ben-albrecht commented Dec 16, 2016 via email

@bradcray
Copy link
Member

It remains unclear to me whether we have a quorum on this decision or whether most people blew it off as being silly and didn't bother responding. I think it'd be worth taking a better straw poll on rather than relying on only those who had time to weigh in on the topic. Or maybe you've got better results than I do...

@daviditen
Copy link
Member

I am fine with @mppf 's proposal.

@noakesmichael
Copy link
Contributor

noakesmichael commented Dec 16, 2016 via email

@mppf
Copy link
Member Author

mppf commented Jan 3, 2017

The proposal has 3 advocates and the only objections are that we have bigger fish to fry.

mppf added a commit that referenced this issue Oct 17, 2018
Rename domain.member and range.member to .contains

* Renames domain.member and range.member to .contains
* Adds deprecation warning versions of these routines
* Updates module code to use .contains
* Updates test code to use .contains

Resolves #5041, which included broader discussion of this direction.

Possible future work: also rename dsiMember to dsiContains.

- [x] full local testing

Reviewed by @ben-albrecht - thanks!
This was referenced Oct 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants