Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Allow to expand until certain condition is met #2790
base: master
Are you sure you want to change the base?
feat: Allow to expand until certain condition is met #2790
Changes from 9 commits
cd96cf9
9f0a224
f691687
7cf4cf8
e8c4ce4
4510332
a3f1588
e0e12a4
96ffae1
7bda685
06aad16
57af3c4
38d9e07
b506e05
b67209a
3c093d4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite following the logic here, but it looks like we're not actually making many changes e.g.
expand
was renamed topopulate_node
should_expand
boolean was extractedget_last_group_node
was moved out ofexpand
Please can you refactor this down to the minimum number of changes necessary. Integration with the existing structure will result in simpler code that is more easily reviewable and maintainable.
I appreciate that we've been through many revisions to get to this point, with complexity from earlier commits removed, however we need to revert some structural changes. Perhaps consider how you'd make these changes from scratch, knowing what you have learned over the course of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree with such approach. I think I was able to cut down amount of changes to the bare minimum.
There is one thing that I would like to discuss - changes in the
recursor
function. Before, when the logic was only to expand everything recursively, the condition was to check ifnode.open==true
(puttingexpansion_count
aside). In my case that is invalid as some nodes have been opened but they shouldn't be traversed further.In both cases reusing
should_expand
function results in correct behavior. First of all, this doesn't have to be true in general. Maybe sometimes the logic forshould_recurse
will be different than the logic forshould_expand
.Second. So, for each node we call
should_expand
twice, once to check if we should expand and second time to check if we should recurse. Between these calls, if shouldshould_expand
returnstrue
, we callexpand
that changes the state of a given node. This means that users must not relay onnode.open
in theirshould_expand
implementation as this would result in incorrect behavior.I don't have enough experience in developing nvim plugins to judge if this is a problem, but wanted to bring this to your attention. Maybe this should at least be mentioned in the documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the consistency, it's more reason-able and will make maintenance easier.
So long as we document that we are providing a consistent user experience.
Is
node.nodes
correct; can they test that?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: guarding against calling this function with a file node