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

Added dont_order_roots option #312

Merged

Conversation

smoyte
Copy link
Contributor

@smoyte smoyte commented May 30, 2018

From the proposed Readme text:

By default, root nodes are also assigned order values globally across the whole database table. So for instance if you have 5 nodes with no parent, they will be ordered 0 through 4 by default. If your model represents many separate trees and you have a lot of records, this can cause performance problems, and doesn't really make much sense.

This PR adds an option for disabling this behavior.

This was proposed in issue #282.

@smoyte
Copy link
Contributor Author

smoyte commented May 31, 2018

Crap, will get those tests fixed and resubmit. Was working locally, hmm.

@smoyte smoyte force-pushed the no_reorder_with_parent_id_nil branch 2 times, most recently from 4d3f3e4 to 31c9e56 Compare May 31, 2018 13:46
@smoyte
Copy link
Contributor Author

smoyte commented May 31, 2018

Gosh, I'm having a real hard time replicating those two spec failures on my local machine. Here is what I tried (for the second one, on 2.3.6):

# Running with same settings as Travis:
rbenv install 2.3.6
rbenv local 2.3.6
export DB=postgresql
export BUNDLE_GEMFILE=$PWD/gemfiles/activerecord_5.2.gemfile
gem install bundler
bundle
bundle exec rake --trace spec:all

# Running one the failing specs a crap ton of times:
for run in {1..50}; do rspec spec/label_spec.rb:304; done

# Running the appropriate suite with the same random seed:
rspec --seed=29271

All the above produce nothing but green.

Any ideas?

@smoyte smoyte force-pushed the no_reorder_with_parent_id_nil branch 2 times, most recently from 025939f to 7d82209 Compare June 1, 2018 13:39
@smoyte
Copy link
Contributor Author

smoyte commented Jun 1, 2018

I ran the specs again last night and two jobs failed again, but they were different jobs!

So something intermittent is going on here. Still can't reproduce on my local.

I just added a bunch of debug outputs and am running the specs again on Travis.

@smoyte smoyte force-pushed the no_reorder_with_parent_id_nil branch from 7d82209 to 29c06c6 Compare June 9, 2018 19:49
@smoyte
Copy link
Contributor Author

smoyte commented Jun 9, 2018

OK I think I found the issue -- I needed to call reload a few times in the test setup. Hopefully all should pass now...

@smoyte smoyte force-pushed the no_reorder_with_parent_id_nil branch from 29c06c6 to a8a41f9 Compare June 9, 2018 19:53
[@a, @c, @d].each(&:reload)

@a.append_sibling(@b)
[@a, @c, @d, @b].each(&:reload)s
Copy link
Collaborator

@mceachen mceachen Jun 10, 2018

Choose a reason for hiding this comment

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

looks like the extra s is a typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How in the name of ...

I swear I ran this locally... anyway incoming...

@smoyte smoyte force-pushed the no_reorder_with_parent_id_nil branch from a8a41f9 to 309c009 Compare June 10, 2018 21:25
@smoyte
Copy link
Contributor Author

smoyte commented Jun 11, 2018

Yes!!!

@mceachen mceachen merged commit 5d44d22 into ClosureTree:master Jun 11, 2018
@mceachen
Copy link
Collaborator

Nice!

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.

2 participants