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

Rails 5.0/5.1: Update to ancestry 3.0.0+ for rails 5.1 fix #18082

Closed

Conversation

jrafanie
Copy link
Member

Rails exists? with empty hash in ancestry 2.2.x was broken by rails PR: 28699

Fixed in ancestry 3.0.0:
stefankroes/ancestry@8c43482

Extracted from #18076

Rails exists? with empty hash in ancestry 2.2.x was broken by rails PR: 28699

Fixed in ancestry 3.0.0:
stefankroes/ancestry@8c43482

Extracted from ManageIQ#18076
@NickLaMuro
Copy link
Member

@kbrock is going to be really happy about this change 😉

Seems good to me. There are some performance improvements in there as well...

Actually, I better make sure that I didn't merge any ancestry patches that should now be removed as a result...

Copy link
Member

@NickLaMuro NickLaMuro left a comment

Choose a reason for hiding this comment

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

Looks like I did have a few patches:

#17360
#17511

Might want to look into making sure these are in place in ancestry-3.0.2, or that these don't have negative impacts being there as a result. My for those PRs testing was definitely against < 3.0.0.

Copy link
Member

@NickLaMuro NickLaMuro left a comment

Choose a reason for hiding this comment

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

Looks like I did have a few patches:

#17360
#17511

Might want to look into making sure these are in place in ancestry-3.0.2, or that these don't have negative impacts being there as a result. My for those PRs testing was definitely against < 3.0.0.

@NickLaMuro
Copy link
Member

(sorry for the duplicate review... forgot to hit "Request changes" on that last one)

@jrafanie
Copy link
Member Author

@NickLaMuro did you want to push changes to remove those patches to my fork? I think it makes sense to have them go together.

@NickLaMuro
Copy link
Member

@jrafanie yeah, I can, but it will most likely take a bit of time for me to re-familiarize myself with the changes I made and what now is updated in ancestry-3.0.2.

@miq-bot
Copy link
Member

miq-bot commented Oct 12, 2018

Checked commit jrafanie@4c38f34 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@kbrock
Copy link
Member

kbrock commented Oct 12, 2018

#17511 is good (the clear_memoized_variables part is probably neutral`

#17360 is a little redundant. I introduced at least 1/2 of that change to the 3.0 branch of ancestry

the way ancestor_ids are calculated is better now (thanks to this PR in general) - think the parsing enhancements/issues have been fixed.

I didn't see the advantages to caching @_parent_id or @_ancestor_ids -- but I remember you got great benefit here.

Do we want to rewrite this patch to just cache those 2 values (plus the clear part)?

@kbrock
Copy link
Member

kbrock commented Oct 25, 2018

ok, so this passes tests. Do we know if this is broken or works?

@jrafanie
Copy link
Member Author

ok, so this passes tests. Do we know if this is broken or works?

This passes but fails with the existing patches for ancestry

See also: 7be4efc

@miq-bot
Copy link
Member

miq-bot commented Nov 6, 2018

This pull request is not mergeable. Please rebase and repush.

@kbrock
Copy link
Member

kbrock commented Nov 6, 2018

Sorry, I jumped the gun on this one LJ ==> #18164

We needed to tweak ancestry_patch

I'm also excited with the changes that got back into ancestry

@kbrock kbrock closed this Nov 6, 2018
@jrafanie jrafanie deleted the rails_5_1_update_ancestry_to_3_x branch July 15, 2022 20:23
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.

4 participants