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

Allow removing the zeroes of durations #1679

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

JKamue
Copy link

@JKamue JKamue commented Jan 22, 2025

Changes

With this merge request the internal removeZeroes function of a Duration will also be accessible publically.

Why?

If you end up having zero values within your duration they are almost impossible to "destruct" with the current state of the Api.
At the moment the only Option is to use rescale(). But this is not compatible / wont help with some of the other use cases offered by the Api.

One concrete example of that is the following:

  1. I want to parse a duration and output it in a way that only displays year, month and days if they are not zero
  2. At the same time I want to aggregate years and months (I dont want 24 months for example)

Try#1 - shiftTo()

Intuitively one would use shiftTo() like this:

// Expectation: {"days": 8, "years": 2}
Duration.fromObject({ months: 24, days: 8 }).shiftTo("years", "months", "days").toObject();
// Reality: {"days": 8, "months": 0, "years": 2}

When using something like toHuman() this will produce an output like 2 years, 0 months, 8 days which I dont want to show to the user. This solution breaks rule#1

Try#2 - rescale()

rescale() is the only function currently capable of removing zero values.

// Expectation: {"days": 8, "years": 2}
Duration.fromObject({ months: 24, days: 8 }).rescale().toObject();
// Reality: {"days": 1, "weeks": 1, "years": 2}

This breaks rule#1 and displays units other than year, month and days .

Try#3 - Best of both worlds?

Duration.fromObject({ months: 24, days: 8 }).rescale().shiftTo("years", "months", "days").toObject();
// {"days": 1, "months": 0, "years": 2}
// shiftTo will always create years and months, even if you only input { days: 8 }

Duration.fromObject({ months: 24, days: 8 }).shiftTo("years", "months", "days").rescale().toObject();
// {"days": 1, "weeks": 1, "years": 2}

Either combination does not work.

Solution with this MR

Duration.fromObject({ months: 24, days: 8 }).shiftTo("years", "months", "days").removeZeroes().toObject()
// {"days": 8, "years": 2}

Adding removeZeroes() at the end will get rid of the leftover 0 months created by shiftTo().
In a real world example using toHuman() would produce a proper output like 2 years, 8 days.

Alternatives

As shown there is no alternative to really get rid of something like 0 months or years if you want to shift to years or months.
There are two possible alternatives but the offer less flexbility to the user:

  1. Add the option to exclude 0 to toHuman()
  2. Add the option to exclude 0 to shiftTo()

But overall I think the ability to remove zeroes whenever wanted warrents the addition of this function.

I am open to feedback, let me know what you think!
I had some troubles generating the docs, so I am happy if you can help me out there.

Copy link

linux-foundation-easycla bot commented Jan 22, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@diesieben07
Copy link
Collaborator

Thank you for your contribution and the thorough analysis!
I agree with your assessment of a dedicated method allowing greater user flexibility and I think that should be kept. However I think also that the use-case you have described is so common for toHuman that it warrants adding an option there. I would suggest showZeros which defaults to true. What do you think?

@JKamue
Copy link
Author

JKamue commented Jan 26, 2025

Thanks for your kind reply!
I agree that most likely removeZeroes().toHuman() would be a common thing to do so the addition as a parameter makes sense and setting showZeroes to true by default will not create any breaking api changes.

I will implement this tomorrow and update the pull request.

Is there other things you think the PR might lack? As mentioned previously I was unable to auto generate the documentation.
Does the code and test coverage fit your expectations?

@JKamue
Copy link
Author

JKamue commented Jan 27, 2025

@diesieben07 I amended better tests for removeZeroes() to the first commit 1d50002 and I added the option to hide zeroes with showZeroes: false when using toHuman() in the second commit ba6a301.

Let me know what you think!

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