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 test documenting #10561, two-digit year parsing #39654

Merged
merged 9 commits into from
Feb 19, 2021

Conversation

jeremiahpslewis
Copy link
Contributor

See issue here: #10561

stdlib/Dates/test/io.jl Outdated Show resolved Hide resolved
@nalimilan nalimilan added dates Dates, times, and the Dates stdlib module test This change adds or pertains to unit tests labels Feb 14, 2021
@jeremiahpslewis
Copy link
Contributor Author

@nalimilan Thank you for the feedback earlier. I believe your comments have been addressed (and the branch has been squashed per general PR requirements). Please let me know if there's anything else you would like to see in this contribution. :)

stdlib/Dates/test/io.jl Outdated Show resolved Hide resolved
stdlib/Dates/test/io.jl Outdated Show resolved Hide resolved
stdlib/Dates/test/io.jl Outdated Show resolved Hide resolved
@jeremiahpslewis jeremiahpslewis requested a review from omus February 16, 2021 19:17
@jeremiahpslewis
Copy link
Contributor Author

@nalimilan Given your earlier review (thanks!), just wanted to see if you had further comments after my corrections (based on @omus' feedback). :)

stdlib/Dates/test/io.jl Outdated Show resolved Hide resolved
@jeremiahpslewis
Copy link
Contributor Author

@omus Thanks for the updated review. I think you make a fair point, with the exception of lpad potentially not doing any work I could grok (I removed it and replaced it with string).

I think that the new test reflects the 'current behavior' of the function and is thus correct and parsimonious. However, we lose as a result the generative intuition of how dataset processing with truncated dates will easily lead to parsing mistakes, which was clearer in my original code. That said, the proper place for the intuition / operator warning is docs / a new default behavior for parsing this type of date and as such beyond the scope of this PR. :)

@omus
Copy link
Member

omus commented Feb 18, 2021

I think you make a fair point, with the exception of lpad potentially not doing any work I could grok (I removed it and replaced it with string).

lpad was just adding a leading zero to 0 and 1. Definitely wasn't essential

@jeremiahpslewis
Copy link
Contributor Author

@omus Ah, now I see it, thanks. Moved strings into the loop and converted to int in the test, think we get best of both worlds there.

Copy link
Member

@omus omus left a comment

Choose a reason for hiding this comment

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

LGTM. Will merge after CI completes

@jeremiahpslewis
Copy link
Contributor Author

jeremiahpslewis commented Feb 19, 2021

Pushed a formatting change (which also triggered a new build).

Copy link
Member

@omus omus left a comment

Choose a reason for hiding this comment

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

It's usually best to keep formatting changes in separate PRs but I'm okay to proceed with them here. Some minor things to change though

stdlib/Dates/test/io.jl Outdated Show resolved Hide resolved
stdlib/Dates/test/io.jl Outdated Show resolved Hide resolved
@omus omus merged commit 62d47af into JuliaLang:master Feb 19, 2021
@jeremiahpslewis
Copy link
Contributor Author

Exciting to see this merged, thanks for your reviews! @omus

ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
…ng#39654)

* Add test documenting JuliaLang#10561, two-digit year parsing

* Update stdlib/Dates/test/io.jl

Co-authored-by: Curtis Vogt <[email protected]>

* Update stdlib/Dates/test/io.jl

Co-authored-by: Curtis Vogt <[email protected]>

* Simplify test behavior

* Simplify test

* Tweak parse direction

* Clean up formatting (and trigger new build)

* Update stdlib/Dates/test/io.jl

Co-authored-by: Curtis Vogt <[email protected]>

* Update stdlib/Dates/test/io.jl

Co-authored-by: Curtis Vogt <[email protected]>

Co-authored-by: Curtis Vogt <[email protected]>
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
…ng#39654)

* Add test documenting JuliaLang#10561, two-digit year parsing

* Update stdlib/Dates/test/io.jl

Co-authored-by: Curtis Vogt <[email protected]>

* Update stdlib/Dates/test/io.jl

Co-authored-by: Curtis Vogt <[email protected]>

* Simplify test behavior

* Simplify test

* Tweak parse direction

* Clean up formatting (and trigger new build)

* Update stdlib/Dates/test/io.jl

Co-authored-by: Curtis Vogt <[email protected]>

* Update stdlib/Dates/test/io.jl

Co-authored-by: Curtis Vogt <[email protected]>

Co-authored-by: Curtis Vogt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dates Dates, times, and the Dates stdlib module test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants