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

Add Indexable#index! overloads with offset parameter #12089

Merged

Conversation

HertzDevil
Copy link
Contributor

Follow-up to #11566. Indexable#index overrides Enumerable#index and has an extra parameter, so it makes sense the raising variant should be overridden in a similar way. (There are further overrides in Indexable's including types, but they do not introduce any new signatures, so this PR will suffice.)

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a discrepancy between the parameter name in code (object) and in the docs (value). #index uses object, so that seems to be the natural choice.

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Jun 3, 2022

The base overload actually uses obj. Maybe we should unify those parameter names here too?

@straight-shoota
Copy link
Member

straight-shoota commented Jul 6, 2022

I'd prefer object over obj and I'd rather standardize on that.

Thumbs up please, if you agree =)

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Jul 6, 2022

No other methods in Enumerable or Indexable use object as a parameter name, but it appears in many container methods, e.g. Array#insert and Set#<< (some similar methods use value instead, which is what the docs here originally put).

On the other hand, obj also shows up in Enumerable#includes? and #each_with_object, plus similar ones in Iterator and Iterable.

@HertzDevil
Copy link
Contributor Author

I don't think it would be wise to do any renamings on the whole standard library in here, so to move forward I suggest using obj here, since that's what the parameter is called in Enumerable#index!, and revert everything else.

@straight-shoota straight-shoota added this to the 1.6.0 milestone Sep 1, 2022
@straight-shoota straight-shoota merged commit a9ce021 into crystal-lang:master Sep 5, 2022
@HertzDevil HertzDevil deleted the feature/indexable-index-bang branch September 6, 2022 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants