TOML: Make Dates a type parameter
#55017
Conversation
This allows the `Dates` calls to be statically inferred
This change also adds back the `.Dates` field, which negates the type stability improvements for now but can be removed once we land a fix to Pkg to use the new internal constructor.
This is another backwards compatibility concession to Pkg, which we'll remove in the breaking follow-up PR.
topolarity
force-pushed
the
ct/toml-stable-dates
branch
from
21c70c8
to
eb02c81
Compare
|
#54755 was only breaking if you accessed internals of TOML, but otherwise should be transparent to the consumer of public APIs |
Doesn't the TOML parser no longer return Dates objects by default, unless you use the new DTParser constructor? |
|
I don't think that type was made public, or was the standard way to call into the package |
TOML.Parser is public and it should parse dates so we have to fix that. |
|
I see we backported #54755 so presumably we have to backport a fix for that... |
| return parser | ||
| end | ||
| # Dates-enabled constructors | ||
| Parser() = Parser{Dates}() |
There was a problem hiding this comment.
Type piracy I guess. Maybe that's ok.
There was a problem hiding this comment.
I left a comment behind in base/toml_parser.jl specifically delegating this definition to the TOML stdlib, so I think this is fine.
I think this change should be backportable for that No type-stability improvements unless we back-port all three PR's, but the other two are optional to backport |
topolarity
force-pushed
the
ct/toml-stable-dates
branch
2 times, most recently
from
e057250
to
842f332
Compare
This will allow us to resolve the
Datesat compile-time eventually. It also fixesTOML.Parser()to return Dates types again.The plan is to land:
Base.TOML.Parser{Dates}for TOML parsing w/ Dates support Pkg.jl#3938This is a breaking change for anyone who was using the internalDatessupport downstream, but then again so was #54755