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

Dir size is the count of children not disk size #104

Closed
wants to merge 6 commits into from

Conversation

lespea
Copy link
Contributor

@lespea lespea commented Aug 10, 2020

Implements #103

Since I was added a new dep, I also sorted/updated the existing deps. If you want me to not do that in this PR I'll revert and just add the new dep.

@lespea
Copy link
Contributor Author

lespea commented Aug 10, 2020

Hmm those tests were failing on my box before I even started working on this pr... I assumed they would pass here but they're still failing. Is this expected?

@bootandy
Copy link
Owner

I think because assert_cmd was upgraded this caused the tests to fail. I would downgrade it for now. I need to get this looked at sometime.

The other thing that catches my eye is that both methods format_string & display_node are now taking a lot of parameters, I wonder if we could put some of them inside 'DisplayData'. I'll pull this branch and have a think about it.

@bootandy
Copy link
Owner

Actually it is because your branch is using slightly more width on the terminal than the existing branch. Yours is probably correct in this case as it is using an extra character of width which the terminal has. But this means several tests are failing the 'exact match' thing as the percent bars are now 1 or 2 chars longer.

bootandy added a commit that referenced this pull request Aug 16, 2020
Merge of
#104
and master branch
@bootandy
Copy link
Owner

Another problem is that this branch loses the 'which directory is biggest color' which is applied normally and makes the biggest
dirs at each level red.

I tried to merge in the above branch with master here:
https://github.com/bootandy/dust/tree/filesize

bootandy added a commit that referenced this pull request Aug 16, 2020
Merge of
#104
and master branch
@lespea
Copy link
Contributor Author

lespea commented Aug 17, 2020

The missing red text I think was a victim of a weird merge. I previously had done some bad math so I fixed that and changed a few tests to assert_eq to get more context in failures. Looks like fixing my math worked. Not sure I have a great idea about all of the params... a bunch of functions take different things so not sure how well abstracting it into a struct would work? I'm open to suggestions!

I had an idea about changing how the tests would be done completely to avoid failures on different machines but it would add so much complexity I'm not sure it's the best idea. I'll think on it some more.

@bootandy
Copy link
Owner

bootandy commented Aug 18, 2020

Ah sorry, I've gone on a refactoring mission on my own branch again and caused conflicts.

I'm going to play around with this idea too.

. Not sure I have a great idea about all of the params

I think display_count should be added to the DisplayData struct.

@bootandy
Copy link
Owner

had an idea about changing how the tests would be done completely to avoid failures on different machines but it would add so much complexity I'm not sure it's the best idea. I'll think on it some more.

What was your idea? This area is tricky, so I'd like to hear it.

I just started copying the files to a '/tmp/' directory - as it was pointed out to me that /tmp dirs usually use the same filesystem but if you run the tests directly the different filesystems will reserve different amounts of disk for small files.

bootandy added a commit that referenced this pull request Aug 18, 2020
Based of #104

Idea is to allow users to find the number of files in each directory
instead of size.
@bootandy
Copy link
Owner

Can I get your thoughts on this merge https://github.com/bootandy/dust/pull/109/files ?

I based some of it of the work you did here. If you think it is a good idea, we can take your commit adding tests and append it to the above merge.

@lespea
Copy link
Contributor Author

lespea commented Aug 19, 2020

I added some comments. Feel free to just close this PR and use yours.

As for my testing idea... there are two potential options.

  1. Use a virtual filesystem. I've never used any in Rust but in my experience in Go they're pretty easy to do and should remove the issue of various filesystems reporting different sizes

  2. Instead of matching the output exactly, basically build a parser around the output and make sure it looks mostly-okay. For example we could ignore the actual size values/bars/percents but maybe see if they're in the expected order or something? Not sure how wildly the values vary and it still might error. Option 1 is probably the more reliable of the two ideas.

@bootandy
Copy link
Owner

Thanks,

Testing

    • I currently copy files to /tmp as that will (usually) be the same across linux distros. I haven't looked into a virtual filesystem but if it works on CI and isn't too complex it could work well.
  1. Some of the tests do only match substrings so they are checking things look sort-of-ok. But I don't want to lose the 'full' tests so I never moved all of them over.
    I think building a full parser is probably going to lead to a bigger project than this is.

But thanks for your ideas anyway.

@skibaa skibaa mentioned this pull request Aug 24, 2020
bootandy added a commit that referenced this pull request Aug 30, 2020
Based of #104

Idea is to allow users to find the number of files in each directory
instead of size.
@bootandy bootandy closed this Aug 30, 2020
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.

2 participants