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

Add leaves instance method #341

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

llenodo
Copy link

@llenodo llenodo commented May 10, 2017

Add an instance method leaves which returns all leaf nodes from a given node.

@coveralls
Copy link

coveralls commented May 10, 2017

Coverage Status

Coverage increased (+0.01%) to 98.023% when pulling becbdcf on llenodo:llenodo/add-leaves-method into dfa3854 on stefankroes:master.

@kbrock
Copy link
Collaborator

kbrock commented May 18, 2017

Currently working towards getting all these methods using scopes instead of moving all records through ruby. Want to hold off on this for just a little bit

@kbrock
Copy link
Collaborator

kbrock commented Jul 5, 2017

@llenodo any ideas how to do this in sql and not ruby?

@kbrock
Copy link
Collaborator

kbrock commented Apr 17, 2018

Also, I think this is going to generate way too many sql calls.

@llenodo
Copy link
Author

llenodo commented Apr 17, 2018

Might be a way to do w/ SQL but would be far more complicated. IMO better to put this in now and refactor later but if you dont like the implementation feel free to reject this PR and create an open issue instead

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.

agreed sql is too much - I'll try and figure out a better test for this

@@ -35,6 +35,11 @@ def test_tree_navigation
assert_equal descendants.map(&:id), lvl0_node.descendant_ids
assert_equal descendants, lvl0_node.descendants
assert_equal [lvl0_node] + descendants, lvl0_node.subtree
# Leaves assertions
leaves = lvl0_node.descendants.select do |node|
Copy link
Collaborator

Choose a reason for hiding this comment

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

ideas how to test this without just rewriting the implementation here?

@kbrock
Copy link
Collaborator

kbrock commented Sep 11, 2019

see also #246

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.

3 participants