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

Implement dirs-only option #95

Closed
wants to merge 7 commits into from

Conversation

klingtnet
Copy link

@klingtnet klingtnet commented Mar 21, 2023

When dirs-only is enabled every node other than a directory will be skipped, i.e. it is not shown in the output.

Closes #84.

TODOs

  • Squash fixup commits

When dirs-only is enabled every node other than a directory will be
skipped, and is not shown in the output.

Closes solidiquis#84.
@@ -89,6 +89,9 @@ impl Tree {
let mut root_id = None;

while let Ok(TraversalState::Ongoing(node)) = rx.recv() {
if ctx.dirs_only && !node.is_dir() {
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm so doing it here would result in the loss of disk usage information which is something I would still want to compute and surface to the end-user unless they supply the --supress-size option. We'd probably want to filter for dirs on the display step which happens somewhere here.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds reasonable, I'll update the PR accordingly.

Copy link
Author

Choose a reason for hiding this comment

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

The condition is now checked inside the display_node function (18e7877). With aeaf10a the unit test got adjusted.

Copy link
Author

Choose a reason for hiding this comment

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

That's the output for the repository:

Screenshot 2023-03-22 at 17 08 05

By the way, is there a way to disable colors in the ouptut? If not I'll create another a issue or PR.

Copy link
Author

Choose a reason for hiding this comment

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

Just saw that this should have printed └─ in front of utils.

Copy link
Owner

Choose a reason for hiding this comment

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

There isn't currently a way to disable colors. Feel free to open an issue for that. I'd likely be the one to address that as unfortunately given how erdtree is built it would require a lot of tedious changes.

Copy link
Author

Choose a reason for hiding this comment

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

I think we need to filter this on a node and not on a display level. I'll notify you when I have a proper fix.

Copy link
Author

Choose a reason for hiding this comment

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

@solidiquis It's not ideal, but pruning the file nodes after the tree has been assembled seems to be the easiest way to get a dirs-only view that also includes size information. To get directory sizes we need to accumulate the size of all nodes below the directory. Commit 3333586 contains the patch that prunes the file nodes if dirs-only is set.

Copy link
Author

Choose a reason for hiding this comment

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

Also, the tree is now properly rendered:

Screenshot 2023-03-23 at 21 19 17

@klingtnet klingtnet requested a review from solidiquis March 22, 2023 16:05
@@ -249,6 +249,9 @@ impl Display for Tree {
ctx: &Context,
f: &mut Formatter<'_>,
) -> fmt::Result {
if ctx.dirs_only && !node.is_dir() {
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps better to move this to the if-statement on line 291 to avoid another making a superfluous function call

Copy link
Author

Choose a reason for hiding this comment

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

I think this is outdated due to #95 (comment).

@klingtnet
Copy link
Author

klingtnet commented Mar 25, 2023

Not sure why the CI fails. Locally it builds and also the tests succeed without any warning, and there's no uncommitted state.

@solidiquis
Copy link
Owner

solidiquis commented Mar 29, 2023

Sorry for taking so long to get back to this as work has been incredibly hectic. I've made quite a few changes since and had to address a bug with pruning here, which influenced how I ended up going about --dirs-only here. Thank you anyhow for pushing for this!

edit: --dirs-only will be included in 1.7.0 later today.

@solidiquis solidiquis closed this Mar 29, 2023
@klingtnet
Copy link
Author

Sorry for taking so long to get back to this as work has been incredibly hectic. I've made quite a few changes since and had to address a bug with pruning here, which influenced how I ended up going about --dirs-only here. Thank you anyhow for pushing for this!

edit: --dirs-only will be included in 1.7.0 later today.

Thank you for the update, I know how stressful it can be to maintain an open source library or application.
Godspeed 👍🏻

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.

Feature idea: folders only
2 participants