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

Remove Hash#key_index #10016

Merged
merged 3 commits into from
Dec 8, 2020
Merged

Remove Hash#key_index #10016

merged 3 commits into from
Dec 8, 2020

Conversation

asterite
Copy link
Member

@asterite asterite commented Dec 2, 2020

As I mentioned in #9952 (comment) I needed this for the compiler and added it to Hash, a really long time ago. I don't think that method is generally useful, and it was only used in one place in the compiler. Ruby doesn't have such method, so let's remove it.

As a side note, I'd also like to remove Hash#first_key and Hash#first_value: they are redundant because one can do hash.first[0] and hash.first[1] (plus they don't exist in Ruby either).

It was added exclusively for the compiler but it's not general enough to
be in the standard library.
@j8r
Copy link
Contributor

j8r commented Dec 2, 2020

If first_key and first_value are removed, last_key and last_value have to be too, and all their nillable ? variants.

@asterite
Copy link
Member Author

asterite commented Dec 2, 2020

Yes. They make the API pretty ugly, in my opinion, for a very little gain.

@straight-shoota
Copy link
Member

Github search showed this method is probably not much in use. Although I use it in crinja, but that can be changed to the same workaround as in the compiler. It should be deprecated instead of direct removal.

Still I'm not 100% convinced that it should really be removed. It is useful not only for the compiler.

@asterite
Copy link
Member Author

asterite commented Dec 2, 2020

Yeah... actually, it is useful, and the other methods too. Now that I think about it, we can use index from enumerable for this, right?

@jkthorne
Copy link
Contributor

jkthorne commented Dec 2, 2020

Should this have a depreciation warning? I am just concerned about upgrading versions and having broken code. I dont know if that is as big of a concern before 1.0.

@asterite
Copy link
Member Author

asterite commented Dec 3, 2020

1.0 will be the last version that will introduce breaking changes. It already includes a bunch of them, I don't think removing this is a big deal. The code will just fail to compile and it's very easy to make it work again.

straight-shoota added a commit to straight-shoota/crinja that referenced this pull request Dec 7, 2020
straight-shoota added a commit to straight-shoota/crinja that referenced this pull request Dec 7, 2020
straight-shoota added a commit to straight-shoota/crinja that referenced this pull request Dec 8, 2020
@asterite asterite added this to the 1.0.0 milestone Dec 8, 2020
@asterite asterite merged commit e3da2a0 into master Dec 8, 2020
@asterite asterite deleted the bug/hash-key-index branch December 8, 2020 12:17
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

4 participants