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.

Sign up for GitHub

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

Jump to bottom

fix: -f work with -T option output empty #1200

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: -f work with -T option output empty #1200

wants to merge 2 commits into from

Conversation

wugeer
Copy link

@wugeer wugeer commented Oct 18, 2024

closes: #1180

cafkafk
cafkafk requested changes Oct 18, 2024
View reviewed changes
Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

Sorry, we don't allow merge commits in PRs, instead try dropping the merge commit and then git pull --rebase origin main in the branch to rebase update instead (or pick rebase from the ui)

@wugeer
Copy link
Author

wugeer commented Oct 19, 2024

Have Done! :)

@wugeer wugeer requested a review from cafkafk October 19, 2024 07:03
cafkafk
cafkafk reviewed Oct 26, 2024
View reviewed changes
Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

This seems to be about 18% slower when testing -l --tree with hyperfine against latest main branch, I haven't had time to look at why yet

@cafkafk
perf: Render::add_files don't check if dir again
860cccf 
The changes introduces a call to the files path checking if it's a
directory, however we store this in the File struct to avoid having
to check more than once.

Using `.path.is_dir()` lead to an 18% runtime increase on `--tree`, using
`.is_directory`() doesn't regress performance, testing on my very noisy
syste, it does even lead to a *slight* performance increase.

Signed-off-by: Christina Sørensen <[email protected]>
cafkafk
cafkafk reviewed Oct 26, 2024
View reviewed changes
Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

I've pushed a performance fix, it would recheck whether the files path was a directory, which isn't nescesarry since that is stored in File anyways.

Can I get a +1 review on this to be sure it's correct/performant?

wugeer
wugeer commented Oct 27, 2024
View reviewed changes
Copy link
Author

@wugeer wugeer left a comment

Choose a reason for hiding this comment

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

Good!

@wugeer
Copy link
Author

wugeer commented Oct 27, 2024

@cafkafk Thank you so much for pointing out my mistakes and offering suggestions for improvement. :)

The performance test results on my local machine are as follows.
before perf:
image

This seems to be about 16% slower when testing -l --tree with hyperfine against v0.20.4,
image
image

after perf
This seems to be about 1% slower when testing -l --tree with hyperfine against v0.20.4. 👍
image
image

Thanks again for your guidance.

@wugeer wugeer requested a review from cafkafk October 27, 2024 03:39
gierens
gierens reviewed Oct 27, 2024
View reviewed changes
Copy link
Member

@gierens gierens left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Especially in nested cases, like when printing our src/ in this repo, this produces a misaligned "tree" with orphan edges. Thinking about it a little more I'm also not too sure we really want -T to work with -f since a file tree only makes sense when there are directories. So to address the linked issue simply making both these options mutually exclusive might be saner solution.

Here an example c r -- -Tf src/:
Screenshot From 2024-10-27 17-51-32

@wugeer
Copy link
Author

wugeer commented Oct 28, 2024

@gierens Indeed, -T signifies that the directory is a key node in the tree structure. If -f is added to filter out directories, the hierarchical structure of the tree may appear somewhat odd in the overall output.
What are your advice on this issue? @cafkafk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gierens gierens gierens left review comments

@cafkafk cafkafk Awaiting requested review from cafkafk

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
Eza Pull-Requests
Status: 🆕 New
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: -f fails to work with -T option
3 participants
@wugeer @gierens @cafkafk