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

Fix arrange/arrange_nodes methods filtering out some nodes. #363

Closed
wants to merge 2 commits into from

Conversation

trafium
Copy link
Contributor

@trafium trafium commented Oct 10, 2017

@trafium trafium changed the title Attempt to fix issue with arrange method filtering out nodes which haven't got it's parent included into input nodes collection (https://github.com/stefankroes/ancestry/issues/362) Attempt to fix issue with arrange method filtering out nodes which haven't got it's parent included into input nodes collection. Oct 10, 2017
@trafium trafium changed the title Attempt to fix issue with arrange method filtering out nodes which haven't got it's parent included into input nodes collection. Attempt to fix issue with arrange method filtering out nodes which haven't got parent included into input nodes collection. Oct 10, 2017
@trafium trafium changed the title Attempt to fix issue with arrange method filtering out nodes which haven't got parent included into input nodes collection. Fix arrange/arrange_nodes methods filtering out some nodes. Oct 10, 2017
@coveralls
Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage decreased (-1.1%) to 96.379% when pulling a77de8f on trafium:master into 8c9ba36 on stefankroes:master.

@vzharkov
Copy link

@trafium this PR break the ordering for the arrange method.

Copy link
Collaborator

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

I think the main thing we need is a failing test, so we don't introduce this again.

It seems very bizarre to have partial trees in the hash. I can see how that was missed.

arranged = ActiveSupport::OrderedHash.new
min_depth = Float::INFINITY
index = Hash.new { |h, k| h[k] = ActiveSupport::OrderedHash.new }
arranged = ActiveSupport::OrderedHash.new
Copy link
Collaborator

@kbrock kbrock Apr 17, 2018

Choose a reason for hiding this comment

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

hashes are ordered by default. can we just use a regular hash?

I think introduced in ruby version 1.9 ~12/2007

@@ -29,27 +29,25 @@ def orphan_strategy= orphan_strategy

# Arrangement
def arrange options = {}
# Get all nodes ordered by ancestry and start sorting them into an empty hash
arrange_nodes self.ancestry_base_class.reorder(options.delete(:order)).where(options)
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure how removing the ordering helps the pr

The issue with removing this ordering is the tree will still get created, but the nodes will not be in the secondary desired order.

Also of note, the ancestry column does not have c collation. So I do not trust sql's order right now.

@kbrock
Copy link
Collaborator

kbrock commented Sep 5, 2018

Thank you for opening this PR.

future suggestion: make a local branch rather than creating this on your master.

I cant merge without a test, or with it breaking existing tests.
I also don't know how we are to figure out how to handle the missing

Please reopen / ping me if you want to work through this

@waqarbaig07 if you have a solution, please share

@kbrock kbrock closed this Sep 5, 2018
@kbrock
Copy link
Collaborator

kbrock commented Nov 1, 2018

@trafium so sorry for closing this.

I pulled this change (a little tweaked) into #415

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.

4 participants