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

Add human-readable sizes option #48

Merged
merged 6 commits into from Apr 22, 2016
Merged

Conversation

jessepeterson
Copy link
Contributor

Implements #16

@jessepeterson
Copy link
Contributor Author

jessepeterson commented Apr 21, 2016

Hah. Totally missed that repoutil has humanReadable(). Will move that to reposadocommon and reference from both repoutil and repo_sync.

else:
return str(cast_unit(round(size_in_bytes/float(limit/2**10), 1))) + suffix


Copy link
Contributor

Choose a reason for hiding this comment

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

Hope nobody is parsing output and relying on the old output format for anything... If the reason for adding a 'HumanReadableSizes' preference was to prevent changing the output format without admin consent, that's pointless with the other changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if you feel strongly about "KiB/MiB/etc", your might consider ditching the 'HumanReadableSizes' preference and just always use human-readable sizes, since the output format is going to change either way.

In any case, if the output format is going to change, it would be best to bring the topic up for discussion on the reposado mailing list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Few things: 1) HumanReadableSizes needs to be explicitly set in your preferences else you get the previous behavior. This means that the only "change" with the KiB/MiB, etc. is in the repoutil --info output. I'm not sure how many folks would be parsing info output. 2) I really don't feel strongly about it. I actually figured you'd comment on that part :), so that whole commit can be reversed if you want. 3) If that commit were reversed would you be averse to changing the units to proper SI units (multiples of 1000) vs. binary prefixes? Output format wouldn't change, but the numbers would, slightly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel strongly about any of this.

Obligatory xkcd: https://xkcd.com/1172/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mostly wanted #16 to go away. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Whereas open issues don't bother me in the least. (unless they affect me)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I reverted the base and now use SI units (in the same format as before). Yay or nay? 😄

@@ -92,6 +92,17 @@ The following keys are optional and may be defined in preferences.plist for spec

Defaults to no log file.

- HumanReadableSizes

Enable human-readable file sizes in download messages e.g. KiB, MiB.
Copy link
Contributor

Choose a reason for hiding this comment

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

Docs don't match current code implementation...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, indeed.

@gregneagle gregneagle merged commit de2fdb6 into wdas:master Apr 22, 2016
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.

None yet

2 participants