Skip to content

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 :)

@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
Contributor

@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
richmahn pushed a commit to unfoldingWord/dcs that referenced this pull request Mar 2, 2025
Fix #32716 Tested, it still works. - cc @wxiaoguang for github/relative-time-element#296 Signed-off-by: Yarden Shoham <git@yardenshoham.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants