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

Add support for CWeek DateTime format #435

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

spariev
Copy link

@spariev spariev commented Dec 30, 2024

Fix for #422

I've added a case clause for cwyear and cweek as per example in #415
Also added a corresponding test

@joshuacronemeyer
Copy link
Collaborator

@spariev this does not handle the scenario where the year is not part of the date string

3.0.1 :006 > Date._parse('W53-6 13:00:01')
 => {:hour=>13, :min=>0, :sec=>1, :cweek=>53, :cwday=>6}

@joshuacronemeyer
Copy link
Collaborator

Haha. Also I forgot to say thank you!!! It's great you're looking at this.

@joshuacronemeyer
Copy link
Collaborator

joshuacronemeyer commented Dec 30, 2024

Oh and one other note we've just had a bug report about the way we handle these kinds of date strings. The way we're just passing in the mocked year into Date.new isn't correct. During the last week of the year the year might need to be the NEXT year. I'm not 100% sure how we're going to fix it but I think it's going to be getting the year from the un-mocked method, computing the year offset of the mocked date and then adding it to the result of the un-mocked method.

#434

@spariev
Copy link
Author

spariev commented Dec 31, 2024

@joshuacronemeyer no problem, thanks for the direct feedback 😂
I'll try fix the case with cweek later this week (doh), your approach seems reasonable enough, maybe we will fix the related bug along the way

@spariev
Copy link
Author

spariev commented Jan 6, 2025

@joshuacronemeyer I see that in #437 there is a fix for cweek as well, so I'll wait until you guys are done with it and then we'll see what to do with this PR.

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