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 duration calculation #296

Merged
merged 4 commits into from
Dec 5, 2024
Merged

Fix duration calculation #296

merged 4 commits into from
Dec 5, 2024

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Dec 4, 2024

Fix duration calculation

The simplest fix for #250 (comment) , not ideal, just a quick fix.

@wxiaoguang wxiaoguang requested a review from a team as a code owner December 4, 2024 16:42
@wxiaoguang wxiaoguang force-pushed the fix-dur branch 2 times, most recently from 3904b3e to 3f93379 Compare December 4, 2024 17:17
@silverwind
Copy link

Maybe a test can be added here:

suite('roundToSingleUnit', function () {

@wxiaoguang
Copy link
Contributor Author

Maybe a test can be added here:

suite('roundToSingleUnit', function () {

If the project members have interests in this PR, I could add later.

@francinelucca
Copy link
Contributor

Thanks so much for diving into this @wxiaoguang! Looks like this proposed solution is causing one test to fail so we're unable to merge it at this time.
If you'd like to take a stab at fixing the tests you're welcome to do so, if not someone from the team will pick this up when we have the availability to do so.

You should be able to run the tests locally using npm run test

@wxiaoguang
Copy link
Contributor Author

Thanks so much for diving into this @wxiaoguang! Looks like this proposed solution is causing one test to fail so we're unable to merge it at this time. If you'd like to take a stab at fixing the tests you're welcome to do so, if not someone from the team will pick this up when we have the availability to do so.

You should be able to run the tests locally using npm run test

But that failure seems not related. I have noticed that even on main branch, it still fails (that's another reason why I didn't touch the test code, I just would like to see whether it fails in your CI tasks).

 ❌ relative-time > [tense=future] > micro formats years
      AssertionError: expected '11y' to equal '10y'
      + expected - actual
      
      -11y
      +10y
      
      at n4.<anonymous> (test/relative-time.js:609:13)

@wxiaoguang
Copy link
Contributor Author

The 11y vs 10y failure is caused by another bug in elapsedTime, const month = Math.floor(day / 30) is not right when the period is long enough.

Copy link
Contributor

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

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

Confirmed CI failure is present in prod, in that case I think we can move forward with this. @wxiaoguang Just have a logic question and adding tests I think is a great idea :)

src/duration.ts Outdated Show resolved Hide resolved
@wxiaoguang
Copy link
Contributor Author

A test is added in fc858c3 and added comments to clarify the problem.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Dec 5, 2024

And made one more commit: if we'd like to support "11 months": 60bc336 corrected the behavior and fixed the tests.

Feel free to choose the behavior you like.

@wxiaoguang
Copy link
Contributor Author

"Fixed" the "10y vs 11y" problem in da833cf, now the CI should be able to pass

Copy link

@viceice viceice left a comment

Choose a reason for hiding this comment

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

🚀 ❤️

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.

This LGTM

@francinelucca francinelucca merged commit bc1b8b7 into github:main Dec 5, 2024
1 check passed
@francinelucca
Copy link
Contributor

Merged! Thanks so much for doing this @wxiaoguang

lunny pushed a commit to go-gitea/gitea that referenced this pull request Dec 5, 2024
Fix #32716

Tested, it still works.

- cc @wxiaoguang for
github/relative-time-element#296

Signed-off-by: Yarden Shoham <git@yardenshoham.com>
@wxiaoguang wxiaoguang deleted the fix-dur branch December 5, 2024 23:09
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.

5 participants