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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only flatten first level to preserve nested #48063

Merged
merged 1 commit into from Apr 25, 2023

Conversation

miharekar
Copy link
Contributor

@miharekar miharekar commented Apr 25, 2023

Motivation / Background

I just tried edge Rails to see what breaks in my app, and this was the first thing that broke 馃槄

I have a hash of hashes for which I use in_order_of, and since this change I don't get ordered array of hashes, but I get a longer array of flattened values.

The actual object:

{:closed=>{:value=>0, :status=>"Closed"},
 :opened=>{:value=>1, :status=>"Opened"},
 :temporarily_closed=>{:value=>2, :status=>"Temporarily closed"},
 :recommended=>{:value=>3, :status=>"Recommended"},
 :declined=>{:value=>4, :status=>"Declined"},
 :submitted=>{:value=>5, :status=>"Submitted"},
 :invited=>{:value=>6, :status=>"Invited"}}

Before:

irb(main):001:0> Cafe.symbolized_status_map.in_order_of(:first, [:opened, :submitted, :recommended, :invited, :temporarily_closed, :closed, :decline
d])
[[:opened, {:value=>1, :status=>"Opened"}],
 [:submitted, {:value=>5, :status=>"Submitted"}],
 [:recommended, {:value=>3, :status=>"Recommended"}],
 [:invited, {:value=>6, :status=>"Invited"}],
 [:temporarily_closed, {:value=>2, :status=>"Temporarily closed"}],
 [:closed, {:value=>0, :status=>"Closed"}],
 [:declined, {:value=>4, :status=>"Declined"}]]

Currently:

irb(main):001:0> Cafe.symbolized_status_map.in_order_of(:first, [:opened, :submitted, :recommended, :invited, :temporarily_closed, :closed, :decline
d])
=>
[:opened,
 {:value=>1, :status=>"Opened"},
 :submitted,
 {:value=>5, :status=>"Submitted"},
 :recommended,
 {:value=>3, :status=>"Recommended"},
 :invited,
 {:value=>6, :status=>"Invited"},
 :temporarily_closed,
 {:value=>2, :status=>"Temporarily closed"},
 :closed,
 {:value=>0, :status=>"Closed"},
 :declined,
 {:value=>4, :status=>"Declined"}]

And it broke, because I'm using it like so:

<% Cafe.symbolized_status_map.in_order_of(:first, [:opened, :submitted, :recommended, :invited, :temporarily_closed, :closed, :declined]).each do |sym, info| %>

and now of course the info is nil 馃槄

This PR fixes it so the behavior stays the same.

Detail

This Pull Request changes improves the implementation of #47805.

Additional information

Asked if this is intended behavior or a broken thing on Discord, and got instructed to open a PR so here we are 馃槃

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@byroot byroot merged commit 6ac6b0b into rails:main Apr 25, 2023
9 checks passed
@miharekar miharekar deleted the fix-nested-in-order-of branch April 25, 2023 20:09
@miharekar
Copy link
Contributor Author

@rafaelfranca Hey, what's the reason this was omitted from the 7.0.5 release?

The #47805 was included but this wasn't so 7.0.5 broke my app 馃槄

@byroot
Copy link
Member

byroot commented May 25, 2023

It's because I didn't backport that commit as it wasn't requested and the PR didn't indicate the issue was existing on 7.0.

I can backport it now.

byroot added a commit that referenced this pull request May 25, 2023
Only flatten first level to preserve nested
@byroot
Copy link
Member

byroot commented May 25, 2023

Done: 90474c6

@miharekar
Copy link
Contributor Author

@byroot yeah, there was no issue on 7.0. The problem got introduced with #47805.

Thanks! 鉂わ笍

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