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

Fix inability to get weeks from Timespan.Humanize #884

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AKTheKnight
Copy link
Contributor

This fixes an issue I saw on SO below:
https://stackoverflow.com/questions/56550059/humanizer-months-weeks-days-hours

Basically you can't seem to get a value like "3 Months, 2 Weeks, 3 Days, 4 Hours" from calling the below:
new TimeSpan(109, 4, 0, 0, 0).Humanize(3, maxUnit: TimeUnit.Month)

This fixes that (and tests that therefore broke) by changing how we calculate Weeks and Days.

Also fixes #862

@SimonCropp
Copy link
Collaborator

@MihaMarkic @hangy @hazzik any thoughts on this one. i have no objections

@MihaMarkic
Copy link
Contributor

@SimonCropp It's a tough one. I see the use for both, but one without weeks might be better because it's shorter and we are more used to it in general. Said that, I'd opt for an option to display one or another as I guess people have different use cases and opinions.

Copy link
Member

@clairernovotny clairernovotny left a comment

Choose a reason for hiding this comment

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

Just need to ensure the CLA is signed and conflicts resolved

@AKTheKnight
Copy link
Contributor Author

I've fixed the conflicts, however unsurprisingly this breaks two tests from different cultures that aren't expecting to suddenly be provided weeks.

Not sure if it would be preferable to make the returning of weeks optional as suggested above, or to make it a breaking change and remove the broken tests/find people who can help fix the test to now get weeks in return

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.

Missing weeks when humanizing TimeSpan with min/maxUnit
4 participants