In Ruby repeated fields, each_index actually iterates over the index #11767
Conversation
|
Oh, hm. Should I not be working off of my fork? From the failing build: I can just branch off this repo, I suppose, if that's more convenient for y'all in the future. |
mkruskal-google
added
the
🅰️ safe for tests
github-actions
bot
removed
the
🅰️ safe for tests
|
@haberman 👋 Hi there! Anything I can do to help move this forward? (No worries if not, just making sure I'm not missing anything.) |
|
HI @shaldengeki, this looks good to me. I think the only question is whether we need to have a major version bump for this. Users could technically be relying on the old buggy behavior. |
|
It's true! This is definitely a breaking change in the API. What does a
major bump entail? (does it just mean "we'll wait to merge this"? happy to
let this sit.)
…On Thu, Feb 16, 2023, 16:25 Joshua Haberman ***@***.***> wrote:
HI @shaldengeki <https://github.com/shaldengeki>, this looks good to me.
I think the only question is whether we need to have a major version bump
for this. Users could technically be relying on the old buggy behavior.
—
Reply to this email directly, view it on GitHub
<#11767 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE6YVH6CPLHYWFQIGOKYKLWX2LNFANCNFSM6AAAAAAUOOLH4E>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@haberman just wanted to check in and set expectations -- let me know if I should be doing anything here. |
|
Thanks for checking in. No action is required on your part, I think we will plan to merge this in coordination with the next major version bump in Ruby, whenever that is (I think it will happen sometime this year). |
|
@haberman apologies if this is noise - I see v24 release candidates are out, should we try to get this in? |
|
@haberman happy Thanksgiving, and touching base once again on this; let me know what I can do to help. |
|
We are planning to bump the major version of Ruby in February. So it's time to move forward with a breaking change like this. Thanks for your patience. |
zhangskz
added
the
🅰️ safe for tests
github-actions
bot
removed
the
🅰️ safe for tests
…11767) Currently we're aliasing `each_index` to `each_with_index`, incorrectly passing both the index and the value of a repeated field to the block. What we want is to just pass the index. Luckily this is a method on Ruby arrays, so we just wrap the native Ruby array method. Fixes #7806 Closes #11767 COPYBARA_INTEGRATE_REVIEW=#11767 from shaldengeki:shaldengeki-repeated-field-each-index-returns-actual-index 874916c FUTURE_COPYBARA_INTEGRATE_REVIEW=#11767 from shaldengeki:shaldengeki-repeated-field-each-index-returns-actual-index 874916c PiperOrigin-RevId: 593820135
…11767) Currently we're aliasing `each_index` to `each_with_index`, incorrectly passing both the index and the value of a repeated field to the block. What we want is to just pass the index. Luckily this is a method on Ruby arrays, so we just wrap the native Ruby array method. Fixes #7806 Closes #11767 COPYBARA_INTEGRATE_REVIEW=#11767 from shaldengeki:shaldengeki-repeated-field-each-index-returns-actual-index 874916c FUTURE_COPYBARA_INTEGRATE_REVIEW=#11767 from shaldengeki:shaldengeki-repeated-field-each-index-returns-actual-index 874916c PiperOrigin-RevId: 593820135
…11767) Currently we're aliasing `each_index` to `each_with_index`, incorrectly passing both the index and the value of a repeated field to the block. What we want is to just pass the index. Luckily this is a method on Ruby arrays, so we just wrap the native Ruby array method. Fixes #7806 Closes #11767 COPYBARA_INTEGRATE_REVIEW=#11767 from shaldengeki:shaldengeki-repeated-field-each-index-returns-actual-index 874916c PiperOrigin-RevId: 593835025
|
This is now announced in https://protobuf.dev/news/2023-12-27/#ruby-breaking-changes |
Currently we're aliasing
each_indextoeach_with_index, incorrectly passing both the index and the value of a repeated field to the block.What we want is to just pass the index. Luckily this is a method on Ruby arrays, so we just wrap the native Ruby array method.
Fixes #7806