search

stefankroes / ancestry

Organise ActiveRecord model into a tree structure
MIT License
3.74k stars 462 forks source link

Materialized path2 #571

Closed kbrock closed 2 years ago

kbrock commented 2 years ago

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 commented 2 years ago

@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?

kshnurov commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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