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.

Sign up for GitHub

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

Jump to bottom

Fix materialized_path2 #597

Merged
merged 2 commits into from
Feb 15, 2023
Merged

Fix materialized_path2 #597

merged 2 commits into from
Feb 15, 2023

Conversation

kshnurov
Copy link
Contributor

@kshnurov kshnurov commented Jan 5, 2023

materialized_path2 was completely broken from the beginning.
This PR fixes it and renames strategy to ancestry_format since it's not really a strategy, it doesn't change any behavior.

@kshnurov kshnurov closed this Jan 5, 2023
@kshnurov kshnurov force-pushed the materialized_path2_fix branch from 0cc6b6d to 8a46ea1 Compare January 5, 2023 11:37
@kshnurov kshnurov reopened this Jan 5, 2023
kbrock
kbrock reviewed Jan 27, 2023
View reviewed changes
Copy link
Collaborator

@kbrock kbrock left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This PR is great.

I do have the goal of going away from all these cattr_accessor values. To be honest, this sounds minor but it has been my main sticking point. It is the reason I am dragging my feet.

Rails has helper methods that mixed into the class that take parameters. And then dynamically creates methods with the special variable (e.g.: delimiter, ancestry_column) passed in as a parameter.

We, on the other hand, mix in a class that access the special variable via cattr.

If we go towards this other behavior, then we can have multiple ancestry columns per record and I believe it would actually simplify our code.

Is there a way to change this code so we are not introducing yet more cattrs?

@kbrock kbrock self-assigned this Jan 27, 2023
@kshnurov
Copy link
Contributor Author

kshnurov commented Jan 27, 2023 •
edited
Loading

@kbrock these parameters were previously hard-coded, which is definitely not good. Now they are used both in other modules (like the materialized_path2 or materialized_path_pg) and application code (like the provided migration).

It probably makes sense to change them to cattr_reader and make some of them private, but other than that it's just defining some variables instead of constants. What's the other option?

@kshnurov
Copy link
Contributor Author

kshnurov commented Jan 29, 2023 •
edited
Loading

@kbrock here's a take on cattr_accessor: https://github.com/kshnurov/ancestry/tree/refactor-options-storage
It has breaking changes: all writers are removed (you shouldn't change ancestry options on-the-fly, that'll lead to unexpected behavior) and all instance accessors are removed.

@kbrock kbrock merged commit f66c906 into stefankroes:master Feb 15, 2023
@kshnurov
Copy link
Contributor Author

@kbrock how about releasing 4.3 on RubyGems? :)

@kshnurov kshnurov mentioned this pull request Feb 24, 2023
Closed
@kshnurov
Copy link
Contributor Author

@kbrock 4.2 is still the latest version on RubyGems, do you have access to release a new version?

@kbrock
Copy link
Collaborator

kbrock commented Feb 24, 2023 •
edited
Loading

@kshnurov Yes, I have access.

moving to issue #603

kbrock added a commit to kbrock/ancestry that referenced this pull request Mar 3, 2023
@kbrock
* Fix: materialized_path2 strategy stefankroes#597
91f100f 
* Fix: descendants ancestry is now updated in after_update callbacks stefankroes#589
* Document updated grammar stefankroes#594
* Documented `update_strategy` stefankroes#588
* Fix: fixed has_parent? when non-default primary id stefankroes#585
* Documented column collation and testing stefankroes#601 stefankroes#607
* Added initializer with default_ancestry_format stefankroes#612
* ruby 3.2 support stefankroes#596
kbrock added a commit to kbrock/ancestry that referenced this pull request Mar 3, 2023
@kbrock
release v4.3 CHANGELOG
db31daa 
* Fix: materialized_path2 strategy stefankroes#597
* Fix: descendants ancestry is now updated in after_update callbacks stefankroes#589
* Document updated grammar stefankroes#594
* Documented `update_strategy` stefankroes#588
* Fix: fixed has_parent? when non-default primary id stefankroes#585
* Documented column collation and testing stefankroes#601 stefankroes#607
* Added initializer with default_ancestry_format stefankroes#612
* ruby 3.2 support stefankroes#596
This was referenced Mar 3, 2023
Merged
Merged
kbrock added a commit to kbrock/ancestry that referenced this pull request Mar 4, 2023
@kbrock
release v4.3 CHANGELOG
a078aa2 
* Fix: materialized_path2 strategy stefankroes#597
* Fix: descendants ancestry is now updated in after_update callbacks stefankroes#589
* Document updated grammar stefankroes#594
* Documented `update_strategy` stefankroes#588
* Fix: fixed has_parent? when non-default primary id stefankroes#585
* Documented column collation and testing stefankroes#601 stefankroes#607
* Added initializer with default_ancestry_format stefankroes#612
* ruby 3.2 support stefankroes#596
@kshnurov kshnurov mentioned this pull request Mar 10, 2023
Closed
This was referenced Mar 10, 2023
Merged
Merged
@kbrock kbrock mentioned this pull request Mar 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbrock kbrock kbrock left review comments

Assignees

@kbrock kbrock

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kshnurov @kbrock