stefankroes / ancestry

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

Drop Orphan strategy class attribute #617

Closed kbrock closed 1 year ago

kbrock commented 1 year ago

This is part of a push to remove so much global state stored in the class. Some parts pulled from #600

orphan_strategy getters and setters on the class were never part of the public API but necessary for an internal implementation. Removing this internal mechanism that has potentially leaked to external implementations.

Currently the supported usage pattern for orphan_strategy is still:

class Model < ActiveRecord::Base
  has_ancestry orphan_strategy: :destroy
end

Dropping accessing and changing the value directly in the class:

class Model < ActiveRecord::Base
  has_ancestry
  # removing: self.orphan_strategy = :destroy
  # removing: puts "changed to #{orphan_strategy}"
end

I did go through github and no projects using ancestry referenced orphan_strategy or orphan_strategy=.

A number of the projects did call has_ancestry with :orphan_strategy, as well as acts_as_tree. Which is good. that means this feature is in use and those projects only use the public api.

kshnurov commented 1 year ago

@stefankroes pay attention to this and please stop breaking this gem.

@kbrock what you've done here is a breaking change and you didn't even mention it in the changelog. First of all, people may have been skipping or overriding apply_orphan_strategy callback, but you've renamed it without any second thought. Also, they could've been using orphan_strategy accessor in other places, but you've just wiped it out.

PLEASE STOP FUCKING UP THIS GEM.

kbrock commented 1 year ago

@kbrock what you've done here is a breaking change and you didn't even mention it in the changelog.

I will be sure to include this change when I do write up the changelog. I've been clear that ever since accepting the hash_options change, this major version will be bumped.

First of all, people may have been skipping or overriding apply_orphan_strategy callback, but you've renamed it without any second thought.

Thank you for pointing this out. I will look into fixing this.

Do you override this method?

If someone didn't want to use our strategy, they can just pass orphan_strategy: none and define their own before_destroy hook. Since this method is not part of the public interface, I didn't realize they would override it. I'm not totally sure what a public interface is now a days since ruby lets you do anything you want.

I see examples of that method being overridden in github:

This is frustrating since I did search github for all ruby projects mentioning orphan_strategy, but the search must change that to \borphan_strategy\b

Also, they could've been using orphan_strategy accessor in other places, but you've just wiped it out.

You can't have it both ways. This was inspired by #600 - that PR drops the orphan_strategy setter and getter. As I stated, I found no code that used this. While that is by no means a thorough investigation of all code that exists, it is a reasonable test. Since the developers are setting this, I didn't see need to tell them what they already know.

PLEASE STOP FUCKING UP THIS GEM.

Really? Do you feel this is the best way to work with another individual? When you contribute to other github projects, do you act this way?

Your tone is so unprofessional, that I am having trouble taking you seriously.

kshnurov commented 1 year ago

Your tone is so unprofessional, that I am having trouble taking you seriously.

I obviously don't want to work with you, because that means fixing your bad code or stopping you from merging it, which I've already done half a dozen times. Your work on this gem in the past month is absolutely unprofessional, junior level code. You're consistently doing things a good programmer or maintainer would never do. I'm done defending this gem, it's such a waste of time.

stefankroes commented 1 year ago

@kshnurov You keep dragging me into this, but you should know that for all intents and purposes, I consider @kbrock the owner of this gem. He has been maintaining it for 7 years without any help from me and without him it would have been dead years ago ❤️

As for me, I think @kbrock has been extremely patient and constructive at every turn. Regardless of what happened and who's right from a technical point of view, your aggressive and negative comments are not wanted here. They should end right here and now.

However, your help and expertise are wanted here very much. I suggest we close this thread. I will delete and/or report any further aggressive or negative messages. You are very much welcome to open new issues or PRs in a positive and constructive manner.

kbrock commented 1 year ago

There is no orphan_strategy: none option and it would not make sense since doing nothing would make ancestry for all the children invalid.

The best way to change the behavior is to extend this hook.

I have already put together a fix but am researching the usage patterns in github to ensure this test reflects the user's needs.

I will also add this method to the public interface / documentation.