search

stefankroes / ancestry

Organise ActiveRecord model into a tree structure
MIT License
3.72k stars 458 forks source link

Refactor options storage #600

Closed kshnurov closed 1 year ago

kshnurov commented 1 year ago

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. We do not mention ancestry_column or ancestry_delimiter accessors in the docs, but if someone is using them this will be a breaking change for them. Since this is an internal API, I'm not sure if we should bump a major version. What do you think?

kbrock commented 1 year ago

I agree that I want these globals removed.

I agree that these are set once in has_ancestry and possibly have default values from a configuration / initializer.

There is a pattern in active record that I would like to follow. There are helper functions that do the actual work in ActiveRecord::Base, e.g: the attribute setter that changes a column value and type casts it. Then a method is dynamically added to the model that has the specifics (e.g.: attribute name) and that calls the helper function.

Ancestry on the other hand sets a class level variable for the name of the attribute and uses a module to add all methods. The attribute name is looked up in the base class.

If we follow the dynamic function and helper method option, then in theory there are no global variables, and we also possibly gain the ability to have multiple trees in the same model.

kshnurov commented 1 year ago

That sounds like a completely different PR and a huge refactoring. This one is just a small improvement, do you have anything against it?

Configuration / initializer - I don't see any value in that, especially if you have multiple models with different ancestry. Having all options set in one place is more convenient IMO.

in theory there are no global variables - ancestry_delimiter, ancestry_column and ancestry_root are useful if you want to do something non-standard with ancestry in your code. I don't think we should hide them.

kbrock commented 1 year ago

Agreed, no major bump.

For me, these are no brainers:

Not sure what we gain from ancestry_options[:orphan_strategy]. I appreciate getting the setters out of there, but not sure going towards a hash really gets us anything.

Maybe if I understand your next step, then it may be more clear

kshnurov commented 1 year ago

There's no next step here. You said you wanted to move away from cattr_accessors, this PR minimizes their count. I don't understand what you don't like about it.

kbrock commented 1 year ago

hi @kshnurov

I like what you did with ancestry_base_class. removing the self and removing #ancestry_base_class. I would like to merge that change right away.

The options hash changes just takes the same data and moves it from a variable to a hash. Not sold that these changes buy us much.

kshnurov commented 1 year ago

The options hash changes just takes the same data and moves it from a variable to a hash. Not sold that these changes buy us much.

It replaces 6 cattr_accessors with 1, isn't that what you wanted? Having all options in one hash is definitely more clean and verbose.

kbrock commented 1 year ago

I would like to take this a little slower. You even concentrated on the ancestry_base_class field in its own 2 commits. Could we just do that in a first PR, and then address the options hash?

kshnurov commented 1 year ago

@kbrock everything is already done in this PR, and you didn't give any objective comment on what's wrong. I'm feeling like I'm wasting my time reasoning with you on some extremely simple and obvious changes.

kbrock commented 1 year ago

fair enough. I can rebase your change to ancestry_base_class and create a PR just for that.

kshnurov commented 1 year ago

@kbrock next time you say you want to get rid of something - don't forget to mention you're not gonna merge such change, so I don't waste my time.

kbrock commented 1 year ago

I'm sorry you feel you have wasted your time. I will restate yet again: I like half of this and am unsure about the other half.

My reasoning behind wanting the accessors out of here was to move them into dynamic method definitions. I stated this. This is an idea that you do not like. I agree with many of your reasons for not liking it. I am still contemplating it.

This change doesn't remove the variables but just changes the way you access them. (Think we disagree on this point, too. But no amount of flaming me will make me change my view on this statement.)

I'm still trying to keep my mind open on the hash idea. Making it difficult for the values to be altered is a good thing.

I just need to remove the noise from the ancestry_base_class first to see how many lines this change will make.

I will reorganize this PR locally to get a better handle on just how much change the hashes introduce.

kbrock commented 1 year ago

status: I am going through this PR.

There are a lot of changes in here that are good, but should not be part of this pr (which focuses on converting cattr_accessors to a frozen hash accessor.

I will pull out these changes into separate PRs and then see where that leads us. I am resolving the conflicts in this PR as I go along.

I now understand what you meant by breaking change. Yes, this is a breaking change as some semi-internal-api methods are changed. While they are internal, I can see the use for them for developers, like when populating bread crumbs.

kbrock commented 1 year ago

Think I would like to release before merging this code.

I am working off of my own version of refactor-options-storage.

and have split out the following concepts:

This is what I meant by dropping them. It may make sense to keep all options in the ancestry_options hash, but I definitely want to removing as much global state as possible.

Also saw ancestry_base_class #620 - would like to come up to a decision this week on this one since the changes are very invasive and it causes conflicts across almost all my branches.

kshnurov commented 1 year ago

@kbrock I guess you have to revert #617 first

kbrock commented 1 year ago

@kbrock I guess you have to revert https://github.com/stefankroes/ancestry/pull/617 first

We can discuss reverting #617 over in that thread. There is time enough to hash that out later. In the meantime, I do not see why that should hold us up here.

Have you seen the differences between 628 and 600? They seem very superficial to me

(Sorry if this is basic for you, but this is what I did to compare the 2 PRs)

# git remote add upstream https://github.com/stefankroes/ancestry.git

# fetch references for the PR code
git fetch upstream
git fetch --force --update-head-ok -u upstream refs/pull/600/head:pr/600
git fetch --force --update-head-ok -u upstream refs/pull/628/head:pr/628
#
git checkout upstream/master -b refactor-options-storage-kshnuvrov
git merge pr/600
git checkout --theirs . # for conflicts, take 600s changes.
git add -u .
git merge --continue # :wq
git diff pr/628

Of course that is a merge and not a rebase, so while it can't be used for a PR, it will give you a quick difference between the 2 PRs and help you decide the best way to move forward.

You can git diff refactor-options-storage-kshnurov to see if you lost any important changes.

But please remember, I need this to be cut up into logical changes.