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 Dates.canonicalize(::Period) #726

Merged
merged 1 commit into from
Oct 22, 2020

Conversation

nickrobinson251
Copy link
Contributor

@martinholters
Copy link
Member

JuliaLang/julia#37391 brings some more tests. Would it make sense to also include them here?

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Oct 19, 2020

JuliaLang/julia#37391 brings some more tests. Would it make sense to also include them here?

🤷‍♂️ I can bring them across if you like? I didn't since one test is enough to test the dispatch this PR adds (and those other tests are testing that the canonicalize logic in Dates produces the expected results). Would you prefer i bring them across to match the linked PR? :)

@martinholters
Copy link
Member

Yes, I think I'd prefer to bring them along. We can easily afford the extra run-time that costs and it would help catch any nasty surprises.

@nickrobinson251
Copy link
Contributor Author

Cool :) i've brought over all the tests from the original PR and rebased to resolve conflicts

@fredrikekre fredrikekre merged commit aaa2e41 into JuliaLang:master Oct 22, 2020
@nickrobinson251 nickrobinson251 deleted the npr/period-canonicalize branch October 22, 2020 17:12
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.

3 participants