-
Notifications
You must be signed in to change notification settings - Fork 10
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
NF: datalad tree command #92
Conversation
…pecify depth parameter)
I updated the command docstring, parameters and examples to reflect the current implementation, so we can use this as a starting point for discussing changes: datalad-next/datalad_next/tree.py Lines 51 to 219 in e32cf58
Yes, for me
I had previously considered distinguishing between two kinds of exclusion filters: 'display+traversal' exclusion filters (=directories will not be yielded nor traversed when searching for datasets) vs. 'display only' exclusion filters (=directories will not be yielded standalone, but may be traversed in the dataset search). For example, with the Is this the point you wanted to address? Or am I missing the mark? |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
… the depth of deepest dataset
…itted) (suggested by @mih)
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.
Alrighty! I think we had a stellar run and this came out beautiful. Thanks for having this kind of stamina!
From my POV we can merge this now without any further changes. We can add the short options now, or in a subsequent PR.
The performance is cool from my POV too -- I think we have more substantial margins for improvement in the rest of datalad, compared to what is done in this PR.
I need to set up the contributor acknowledgement framework we use in other extension too, to acknowledge you properly for this contribution -- I will get to that soonish!
Thanks much! I love it!
It was my pleasure @mih! Thank you for the feedback and guidance throughout. Glad to have the chance to learn a bit about datalad internals, too (well, more like took a peek underneath the API). Debugging tips also invaluable for daily usage / life. I will rename the options and add short variants as parts of this PR. If you or the team have any improvement suggestions (also on documentation, code style / naming conventions, refactoring the tests a bit, etc) I'm happy to follow up on separate PRs. |
Huh, one of the datalad-core tests started failing. It seems to be an issue with git-annex. I do not see an immediate connection. Will investigate.... I am rerunning the tests on the |
Yeah, unrelated and should be gone with the 0.17.3 release that just came out. |
I just saw that a changelog snippet was still missing, so I added one with |
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.
Good to go!
New command
datalad tree
for displaying directory/dataset hierarchies.Closes #78
Note:
datalad tree
command? #78, I have included only long-form parameters for now. I think it would be worth considering short-form variants as well, to make it feel similar totree
. My suggestion would be-L
for--depth
(consistent withtree
syntax) and-R
for--dataset-depth
(consistent with--recursion-limit
in other datalad commands).As a first-time contributor, this was really fun! The development environment (including tests and building docs) was straightforward to set up following the instructions. I was also able to reuse helpers in the utils modules (e.g. for creating test directory trees) which saved me lots of time.
Looking forward to your review! Please be brutal :-)