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

Materialized path2 #571

Merged
merged 3 commits into from
Apr 29, 2022
Merged

Materialized path2 #571

merged 3 commits into from
Apr 29, 2022

Conversation

kbrock
Copy link
Collaborator

@kbrock kbrock commented Jan 6, 2022

This introduces another serialization technique for the ancestry column

Will probably be converting these methods over to helper methods that do not call ancestry_column, so I'm not sure if introducing this helps or just introduces more gook to change

kbrock added 3 commits January 5, 2022 21:17
materialized path uses ancestry.nil? to be the root
This puts that logic into a variable

trying hard to remove the assumptions about the ancestry column:
- ancestry.nil? means something
- field is an empty string to mean no ancestors
Before:
ancestry = root? ? nil : ancestor_ids.join("/")

After:

Implications:

ancestry == "#{parent.ancestry}#{parent.id}/" # always

query for my children/descendants no longer has an OR (big win)
no more IS NULL sorting shenanigans
@kbrock
Copy link
Collaborator Author

kbrock commented Jan 25, 2022

@d-m-u you have a comment on whether this is just adding complexity now or you think it moves us forward on allowing plug and play materialized path strategies?

@kbrock kbrock merged commit 946f20d into stefankroes:master Apr 29, 2022
@kbrock kbrock deleted the materialized_path2 branch April 29, 2022 14:01
kbrock added a commit to kbrock/ancestry that referenced this pull request Jun 10, 2022
* added strategy: materialized_path2 stefankroes#571
* Added tree_view method stefankroes#561 (thx @Bizcho)
* Fixed bug when errors would not undo callbacks stefankroes#566 (thx @daniloisr)
* ruby 3.0 support
* rails 7.0 support (thx @chenillen, @petergoldstein)
* Documentation fixes (thx @benkoshy, @mijoharas)
@kbrock kbrock mentioned this pull request Jun 10, 2022
@kshnurov
Copy link
Contributor

I guess this one wasn't tested? It's not even loaded, you can't inherit a module (syntax error, unexpected '<' (SyntaxError) module MaterializedPath2 < MaterializedPath), the proper ROOT = '/' is not set in materialized_path2, and some tests will inevitabely fail with materialized_path2.

@kshnurov
Copy link
Contributor

Fixed most (all?) of the issues in this branch.
Failing tests should be updated or removed (like the ones that use ancestry=, I think this method should be private).

In my opinion, the new strategy is better because it uses 1 condition instead of 2.

@kbrock
Copy link
Collaborator Author

kbrock commented Sep 13, 2022

Yea, I just found a few errors in here while I was refactoring something. Think it ends up with one too many slashes too

I had run a bunch of tests on this but looks like I botched it when moving it across

@kshnurov
Copy link
Contributor

Waiting for you to approve this PR, and I'll send a fix for this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants