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

Dates: Error on construction/parsing with empty strings #47117

Merged

Conversation

anowacki
Copy link
Contributor

@anowacki anowacki commented Oct 9, 2022

This PR is an RFC relating to the discussion in #43883. It is
meant to be used to help in making the decision whether the change
can be made in a minor Julia release, or really does need to
wait for a major version change. The commit message follows:


When attempting to construct a DateTime, Date or Time from an
AbstractString, throw an ArgumentError if the string is empty.
Likewise, error when parseing an empty string as one of these types,
and return nothing from tryparse.

This behavior differs from previously. Before, Date and Time would
return default values of Date(1) and Time(0), respectively, while
DateTime would error without a format argument. With a format
argument, it would return DateTime(1). However, this appears not to
have been explicitly intended, but rather a consequence of the way
parsing was implemented; no tests for empty string parsing existed.

This addresses #28090 and #43883; see discussion therein.

Summary of changes:

  • Check for empty string in Base.parse(::DateTime) and throw if so.
  • Change documentation to mention this.
  • Add a compat notice to the docs contrasting the old behavior.

When attempting to construct a `DateTime`, `Date` or `Time` from an
`AbstractString`, throw an `ArgumentError` if the string is empty.
Likewise, error when `parse`ing an empty string as one of these types,
and return `nothing` from `tryparse`.

This behavior differs from previously.  Before, `Date` and `Time` would
return default values of `Date(1)` and `Time(0)`, respectively, while
`DateTime` would error without a `format` argument.  With a `format`
argument, it would return `DateTime(1)`.  However, this appears not to
have been explicitly intended, but rather a consequence of the way
parsing was implemented; no tests for empty string parsing existed.

This addresses JuliaLang#28090 and JuliaLang#43883; see discussion therein.

Summary of changes:
- Check for empty string in `Base.parse(::DateTime)` and throw if so.
- Change documentation to mention this.
- Add a compat notice to the docs contrasting the old behavior.
@anowacki anowacki force-pushed the an/dates-empty-string-parsing branch from 40e1d33 to a0a9988 Compare October 9, 2022 22:47
@KristofferC
Copy link
Member

@nanosoldier runtests(ALL, vs = ":master")

@kshyatt kshyatt added the dates Dates, times, and the Dates stdlib module label Oct 10, 2022
@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@anowacki
Copy link
Contributor Author

I'm not experienced in reading the output of PkgEval, but I can't personally see anything Dates-related in any of the failures. (Parsers.jl has Dates-related tests which still pass.) So it may be safe to say no registered packages have tests which rely on parsing empty strings as valid Dates, Times or DateTimes.

@gbaraldi
Copy link
Member

It seems Parsers.jl already errored on these cases so I doubt there is much of a change there. I get EOF errors.

@quinnj
Copy link
Member

quinnj commented Oct 10, 2022

It seems Parsers.jl already errored on these cases so I doubt there is much of a change there. I get EOF errors.

Parsers.jl has it's own re-implementation of Date parsing since the last major version release, so no issues there.

@anowacki
Copy link
Contributor Author

Actually, testing all the 'failing' packages locally with this PR shows them passing, so I'm confident the failures are totally unrelated.

@PallHaraldsson
Copy link
Contributor

So just merge this?

@anowacki
Copy link
Contributor Author

@KristofferC anything more needed to make a decision here, or are discussions ongoing? I'm happy to help in any way.

@JeffBezanson JeffBezanson merged commit 317211a into JuliaLang:master Nov 10, 2022
KristofferC pushed a commit that referenced this pull request Nov 13, 2022
When attempting to construct a `DateTime`, `Date` or `Time` from an
`AbstractString`, throw an `ArgumentError` if the string is empty.
Likewise, error when `parse`ing an empty string as one of these types,
and return `nothing` from `tryparse`.

This behavior differs from previously.  Before, `Date` and `Time` would
return default values of `Date(1)` and `Time(0)`, respectively, while
`DateTime` would error without a `format` argument.  With a `format`
argument, it would return `DateTime(1)`.  However, this appears not to
have been explicitly intended, but rather a consequence of the way
parsing was implemented; no tests for empty string parsing existed.

This addresses #28090 and #43883; see discussion therein.

Summary of changes:
- Check for empty string in `Base.parse(::DateTime)` and throw if so.
- Change documentation to mention this.
- Add a compat notice to the docs contrasting the old behavior.
@anowacki anowacki deleted the an/dates-empty-string-parsing branch November 14, 2022 12:00
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants