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

TOML: Return Dates objects by default, even for explicit Parser #55020

Closed

Conversation

topolarity
Copy link
Member

The doc-comment here strongly suggests this was intended to be a public interface (although not the most frequently used one), so this tries a bit harder to avoid breaking the Dates-related behavior here.

This removes DTParser, which effectively behaved the way the existing Parser() behaved before #54755

As a side effect, this hides the Base.TOML.Parser type out of the way the public API.

The doc-comment here strongly suggests this was intended to be a public
interface (although not the most frequently used one), so this tries a
bit harder to avoid breaking the Dates-related behavior here.

This removes `DTParser`, which effectively behaved the way the existing
`Parser()` behaved before JuliaLang#54755

As a side effect, this hides the `Base.TOML.Parser` type out of the way
the public API.
@topolarity topolarity requested review from vtjnash and KristofferC July 3, 2024 22:50
topolarity added a commit to JuliaLang/Pkg.jl that referenced this pull request Jul 3, 2024
This behavior had actually subtly changed with JuliaLang/julia#54755
so that this no longer returned Dates objects, but sticking to the
public `TOML.Parser` interface should prevent that from being an issue
after JuliaLang/julia#55020 lands.
topolarity added a commit to JuliaLang/Pkg.jl that referenced this pull request Jul 3, 2024
This behavior had actually subtly changed with JuliaLang/julia#54755
so that this no longer returned Dates objects, but sticking to the
public `TOML.Parser` interface should prevent that from being an issue
after JuliaLang/julia#55020 lands.
topolarity added a commit to JuliaLang/Pkg.jl that referenced this pull request Jul 3, 2024
This behavior had actually subtly changed with JuliaLang/julia#54755
so that this no longer returned Dates objects, but sticking to the
public `TOML.Parser` interface should prevent that from being an issue
after JuliaLang/julia#55020 lands.
@KristofferC
Copy link
Member

Will it be confusing if TOML.Parser() doesn't return a TOML.Parser. Say:

struct Cache
    parser::TOML.Parser
    ....
end

I wonder if the only way to do this "properly" is to have two Parser structs, one in Base, one in TOML, both <: AbstractTOMLParser and compile the toml parsing code once for each of them.

@topolarity
Copy link
Member Author

OK, I think I have a plan that'll work:

  1. Add the Parser{Dates} type parameter in this change, but don't delete the .Dates field (to avoid breaking Pkg). TOML.Parser() will return a Parser{Dates} which isa TOML.Parser
  2. Change Use Base.TOML.Parser{Dates} for TOML parsing w/ Dates support Pkg.jl#3938 to use Base.TOML.Parser{Dates}()
  3. Once Pkg is updated, remove the .Dates field in TOML: Improve type-stability #55016

Let's handle (1) in #55017 and I'll close this PR for now.

@topolarity topolarity closed this Jul 4, 2024
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