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

Reimplement duration rounding #301

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

leduyquang753
Copy link
Contributor

@leduyquang753 leduyquang753 commented Dec 31, 2024

(This is part of a set of multiple pull requests looking to overhaul the calculation functions.)

This pull request reimplements the roundToSingleUnit function with more robust logic. The original date is now computed through the applyDuration function (a new implementation is in #298).

As this is an isolated pull request, tests will appear to fail until changes from the other pull requests of the set are integrated.

@leduyquang753 leduyquang753 requested a review from a team as a code owner December 31, 2024 10:30
@leduyquang753
Copy link
Contributor Author

I have moved the implementation of relativeTime to apply to roundToSingleUnit as per @keithamus's suggestion in #300.

Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

I think this is looking good 👍. There are some failing tests which need to be addressed please, but overall I like the shape of this.

@leduyquang753
Copy link
Contributor Author

leduyquang753 commented Jan 13, 2025

As this is an isolated pull request, tests will appear to fail until changes from the other pull requests of the set are integrated.

This is expected and I have mentioned in my new pull request description: "As this is an isolated pull request, tests will appear to fail until changes from the other pull requests of the set are integrated."

Particularly, the tests are based on the usage of the new implementations of applyDuration and elapsedTime from the other pull requests.

@keithamus
Copy link
Member

This is expected and I have mentioned in my new pull request description

That's fine, more meant as a note for colleagues. I'd like us to merge in an order which maintains the health of the main branch, if we can (despite the current failing test in main).

the other pull requests

Which other PRs do you anticipate will reduce the test failures on this one?

@leduyquang753
Copy link
Contributor Author

Which other PRs do you anticipate will reduce the test failures on this one?

My new implementation of applyDuration in #298 and of elapsedTime in #299.

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