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

Fix Indexable.range_to_index_and_count to not raise IndexError #10191

Merged

Conversation

straight-shoota
Copy link
Member

Fixes #10170

@Sija
Copy link
Contributor

Sija commented Jan 3, 2021

I'd add instead nilable Indexable.range_to_index_and_count?

@straight-shoota
Copy link
Member Author

That'd be more complicated for splatting the tuple.

@asterite
Copy link
Member

asterite commented Jan 6, 2021

The problem with this is that now this method is inlined in every other method. Previously it was a different call. Can we maybe make two versions, one with a question mark? And a third one that yields.

@straight-shoota
Copy link
Member Author

Is inlining so bad here?

@asterite
Copy link
Member

asterite commented Jan 6, 2021

Indexable.range_to_index_and_count is 20 lines long. It will make those 20 lines appear in every array instance, and that's generic.

I just read @Sija 's suggestion, that's what I'd do to (and it's how it's done everywhere else)

@straight-shoota
Copy link
Member Author

I let Indexable.range_to_index_and_count return nil and used inline return/raise which works pretty fine 👍

@Sija
Copy link
Contributor

Sija commented Jan 6, 2021

@straight-shoota It won't be needed to add all this boilerplate code (raise IndexError.new) if you'd add a nilable method version.

@straight-shoota
Copy link
Member Author

return nil is still necessary, so I'd leave it like that and use a single method for both variants.

@straight-shoota straight-shoota added this to the 1.0.0 milestone Jan 6, 2021
@asterite asterite merged commit 524c52e into crystal-lang:master Jan 7, 2021
@straight-shoota straight-shoota deleted the fix/string-index-negative-range branch January 7, 2021 15:14
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.

Negative range in nullable string brackets raises an out of bounds exception
3 participants