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

Deprecate Enumerable#grep #8452

Merged
merged 1 commit into from Nov 14, 2019
Merged

Conversation

@j8r
Copy link
Contributor

j8r commented Nov 7, 2019

Enumerable#grep aliases Enumerable#select, which has already multiple overloads.

Reasons of the change:

  • select is more widely used, grep is only present in this very method
  • select is a more descriptive English word about the action performed than grep
  • follows the least aliases principle

Forum thread: https://forum.crystal-lang.org/t/remove-enumerable-grep/1323

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Nov 7, 2019

I would personally do it the other way around: use grep, remove select with pattern, remove reject with pattern, add grep_v. But that's just my opinion.

@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Nov 7, 2019

Keeping select sounds more consistent too me as well.

The inverse operation is reject(pattern). We'd have to rename that overload as well, or accept having differently named counterparts for reject.

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Nov 7, 2019

I'm mainly against select because of this:

ary = [1, 6, 2, 4, 8]
ary.select!(2..4) # what do you expect?
@Sija

This comment has been minimized.

Copy link
Contributor

Sija commented Nov 7, 2019

@asterite explanation you've given in https://forum.crystal-lang.org/t/can-anyone-explain-range-as-array-select-arguments-to-me/1320/7 makes a perfect sense to me, imo once you know that #select operates on values then there's not much room for confusion left anymoar.

Although I have to say that #grep(2..7) is more expressive to me than #select(2..7), mainly because of how you'd grep for things in opposite to selecting sth - which might be an index.Using #select OTOH hand is more consistent with no-aliases rule of Crystal.

Option no. 3 would be to leave it as-is I guess.

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Nov 7, 2019

once you know that

I think that's the main issue. People will face the issue, learn that it doesn't work as they expect it, then learn and move on. But I'd rather have something that is intuitive right from the go.

@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Nov 7, 2019

I understand that there is some potential for misinterpretation. But it only affects the rather uncommon case when value and index type are identical or at least similar.
And IMO that's less confusing than having methods with two different names for the same behaviour, and the already mentioned inconsistency with reject.

I guess another option would be to rename #select to #filter, which is another typical name for this behaviour (both are aliases in Ruby's Enumerable for example). filter better expresses that it operates on values. But that would probably also a pretty bold change at this stage...

@j8r

This comment has been minimized.

Copy link
Contributor Author

j8r commented Nov 7, 2019

grep is also confusing, because it doesn't behave as UN*X's grep, which match for a string, with optionally wilcards, not a regex-like pattern.
The method would need to be egrep, or grep_e, to fit the logic perfectly.

It's a bad idea to reproduce UN*X commands, because the methods are behaving differently from them.

Aliases Enumerable#select, which has already multiple overloads
@j8r j8r force-pushed the j8r:deprecate-enumerable-grep branch from 893db68 to 7dc6c34 Nov 7, 2019
@RX14
RX14 approved these changes Nov 14, 2019
@RX14 RX14 merged commit 7a86e8c into crystal-lang:master Nov 14, 2019
6 checks passed
6 checks passed
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32_std Your tests passed on CircleCI!
Details
ci/circleci: test_preview_mt Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@RX14 RX14 added this to the 0.32.0 milestone Nov 14, 2019
@j8r j8r deleted the j8r:deprecate-enumerable-grep branch Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.