Skip to content

refactor: use humantime and humansize for humanly units#25

Closed
nc7s wants to merge 1 commit intoGitoxideLabs:mainfrom
nc7s:migrate-human-deps
Closed

refactor: use humantime and humansize for humanly units#25
nc7s wants to merge 1 commit intoGitoxideLabs:mainfrom
nc7s:migrate-human-deps

Conversation

@nc7s
Copy link

@nc7s nc7s commented Sep 26, 2023

Currently compound_duration and human_format are used. They don't see much use like the replacements, and the former has a 32-bit problem.

Another reason is that I, as a member of the Debian Rust team, hope to introduce less Rust crates into Debian, especially the less used ones. Packaging is more or less a manual process, unlike cargo publish which is automagic.

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

If it helps you, I am all for it!

Could you rewrite the commit to be refactor!: … to indicate it's a breaking change?
Thanks for taking a look at my other comment as well.

assert_eq!(
format!("{}", unit.display(100_002, Some(7_500_000), None)),
"100.0k/7.5M objects [1%]"
"100.0kB/7.5MB objects [1%]"
Copy link
Member

Choose a reason for hiding this comment

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

It looks like it now treats the amount of objects as amount of bytes. Can this be made so it's the same as before?
With none of these changes there should be a change in unit.

Copy link
Author

Choose a reason for hiding this comment

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

My bad, didn't realize it's general units instead of specific sizes. I'll figure it out.

@nc7s
Copy link
Author

nc7s commented Sep 27, 2023

Could you rewrite the commit to be refactor!: … to indicate it's a breaking change? Thanks for taking a look at my other comment as well.

Oh I thought it's only an internal change so not breaking, but sure.

@Byron
Copy link
Member

Byron commented Sep 27, 2023

You don't seem convinced, so I wasn't convinced anymore either as I just looked at the diff before. Now I checked more thoroughly and can confirm that human.rs is a publicly accessible module that publicly exposed types of an external crate that is now a different one.

@nc7s
Copy link
Author

nc7s commented Sep 27, 2023

It's not that I'm not convinced; my initial thoughts writing the change were "it's only replacing dependencies, no publicly visible changes", and my brain when replying was still booting up from sleep. You are right about human.rs.

alexanderkjall added a commit to alexanderkjall/prodash that referenced this pull request Oct 24, 2023
@Byron
Copy link
Member

Byron commented Oct 24, 2023

With #26 merged, would this PR be superseded? To me it seems like it can be closed (or needs work). Thank you.

@Byron Byron added the question Further information is requested label Oct 24, 2023
@nc7s
Copy link
Author

nc7s commented Oct 24, 2023

With #26 merged, would this PR be superseded? To me it seems like it can be closed (or needs work). Thank you.

Indeed. humansize is by name only for sizes, it would be inappropriate to jury rig it here anyway. Sorry for the delay - caught in work.

@nc7s nc7s closed this Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

question Further information is requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants