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

Diff view now shows folder sizes as sum of sizes of contained items #1156

Merged
merged 1 commit into from
Jan 4, 2022

Conversation

skrap
Copy link
Contributor

@skrap skrap commented Jan 3, 2022

I found that I wanted a diff view which would allow me to investigate unexpectedly large backups. Since the size column was unused on the current diff view, and it seemed natural to me to have it be the sum of the size of the contained items, I made that change.

String compare is used when filtering because large diffs were taking too long with os.path.commonpath, which is more general than what we need here.

@m3nu
Copy link
Contributor

m3nu commented Jan 3, 2022

Awesome! We had an open issue for this actually.

@m3nu
Copy link
Contributor

m3nu commented Jan 3, 2022

Fixes #1140

Copy link
Contributor

@m3nu m3nu left a comment

Choose a reason for hiding this comment

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

This is a useful addition and I didn't notice any performance problems. Might start using the diff feature actually. 😄

Any objections or comments, @Hofer-Julian ? Since this feature is your baby.

@m3nu
Copy link
Contributor

m3nu commented Jan 4, 2022

We also have #1138 to add sorting by size for diffs. It adds more code, but isolated in one class. And the contributor didn't adjust the tests yet.

@m3nu m3nu requested a review from Hofer-Julian January 4, 2022 16:57
Copy link
Collaborator

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

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

I have no machine available to test it at the moment.
However, the code change is simple enough and the use case legit.
Thanks!

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.

3 participants