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: missing handling of week on manipulating duration #2583

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

Conversation

adlanarifzr
Copy link

@adlanarifzr adlanarifzr commented Feb 17, 2024

close #2582

@adlanarifzr adlanarifzr marked this pull request as draft February 17, 2024 05:47
@adlanarifzr adlanarifzr marked this pull request as ready for review February 17, 2024 05:53
@vad99lord
Copy link

vad99lord commented Apr 12, 2024

Does this pr address #2479 , #2464?

Looks like the bug was introduced here: #2337

It has worked for my use case with patching based on this pr edit

@manchicken
Copy link

does this pr address #2479 , #2464?

I hope so? I’d love to be able to update.

@iamkun
Copy link
Owner

iamkun commented Apr 28, 2024

Thanks, can you please fix the failed unit test before this PR gets merged?

@vad99lord
Copy link

vad99lord commented Apr 28, 2024

@iamkun, could you please clarify what is the logic behind as and get methods in the duration plugin?

Is my understanding correct that as converts the whole duration to the specified unit, ie how many weeks does this instance represent?
And get retrieves only a part of the duration relevant to the instance unit, i.e I've provided "2 weeks" as part of the initial duration string => the method returns 2?

I'm not quite sure that I understand get method for weeks fully, because currently there seems to be a specific case for the weeks that basically makes this method act as as converting the whole duration in terms of weeks.

There is also a difference in parsing between milliseconds input not using weeks and other types of inputs accepting weeks.

Overall, It seems that there should be weeks stored in any parsing so that there is no extra logic for them except formatting, but I may be mistaken.

update:

opened a new pr #2648 implementing this logic.

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.

Dayjs subtract does not support week based dayjs durations => expected?
4 participants