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

Migrate toml to be on toml_edit #340

Closed
4 tasks done
epage opened this issue Sep 23, 2022 · 20 comments · Fixed by #475
Closed
4 tasks done

Migrate toml to be on toml_edit #340

epage opened this issue Sep 23, 2022 · 20 comments · Fixed by #475
Labels
A-serde Area: Serde integration C-enhancement Category: Raise on the bar on expectations

Comments

@epage
Copy link
Member

epage commented Sep 23, 2022

toml will be a wrapper around the toml_edit crate, providing serde integration, decoupled from the format-preserving parsing, and offering a more stable API

As part of switching cargo adopting toml_edit, toml_edit::easy, a copy of tomls API, was created so cargo could have a single toml parser for both serde and editing.

We plan to move toml_edit::easy into the toml crate, providing nearly the same API at nearly the same performance while reducing overall maintenance effort and allowing people who have both needs to have a common parser.

Blockers

Performance

Compatibility

@epage epage added the C-enhancement Category: Raise on the bar on expectations label Sep 23, 2022
@epage epage added the A-serde Area: Serde integration label Sep 23, 2022
@epage epage pinned this issue Sep 23, 2022
This was referenced Sep 23, 2022
@NobodyXu
Copy link

From comment on #327:

We plan to move toml_edit::easy into the toml crate, providing nearly the same API at nearly the same performance while reducing overall maintenance effort and allowing people who have both needs to have a common parser.

We plan to move toml_edit::easy into the toml crate, providing nearly the same API at nearly the same performance while reducing overall maintenance effort and allowing people who have both needs to have a common parser.

We do not want toml_edit in tomls public API as toml is easier to maintain compatibility on and I'd like for it to be something people can rely on not changing. Or in other words, I want to take toml to 1.0 soon after this change over happens.

So there will be another crate that does the parsing, which is used by both toml and toml_edit?

@epage
Copy link
Member Author

epage commented Dec 23, 2022

toml_edit will be the parser crate. Just toml_edit::easy will change (be removed) and instead it will be in the toml crate, calling into toml_edit.

@NobodyXu
Copy link

toml_edit will be the parser crate. Just toml_edit::easy will change (be removed) and instead it will be in the toml crate, calling into toml_edit.

Thanks for the explanation, though I am a bit worried about the extra compilation time and the increase in binary size caused by this.

I just replace toml_edit with toml in cargo-binstall and that removes 3 dependencies while reducing the size of the debug build cargo-bins/cargo-binstall#610 , but if toml is to use toml_edit, I guess it will be no difference from just keep using toml_edit.

@epage
Copy link
Member Author

epage commented Dec 23, 2022

We can't say much about either until #327 is complete but once it is, I'd recommend trying it out and reporting back (as new issue) concrete build time and release binary size numbers for us to evaluate if the impact is bad enough and what we can do about it.

I say release binary size as that is what I expect most end-users will care about (that is what is generally distributed). If you have a use case for debug binary size, feel free to also share that.

@NobodyXu
Copy link

We can't say much about either until #327 is complete but once it is, I'd recommend trying it out and reporting back (as new issue) concrete build time and release binary size numbers for us to evaluate if the impact is bad enough and what we can do about it.

Sure

I say release binary size as that is what I expect most end-users will care about (that is what is generally distributed). If you have a use case for debug binary size, feel free to also share that.

TBH, I also don't care about debug binary size much, I just use it to check whether a PR might increase the final release build.

@elpiel
Copy link

elpiel commented Dec 23, 2022

I'd like to join in the conversation by saying that debug builds will matter for constraint machines.

It seems that the crate can be made no_std so it still makes sense not to bloat the size.

messense added a commit to messense/ruff that referenced this issue Jan 6, 2023
The `toml` crate doesn't support TOML 1.0, but `toml_edit` does.
While there is a plan to [migrate `toml` to be on `toml_edit`](toml-rs/toml#340),
it's not ready yet and it's very easy to switch back to `toml` when
it's ready.
charliermarsh pushed a commit to astral-sh/ruff that referenced this issue Jan 6, 2023
The `toml` crate doesn't support TOML 1.0, but `toml_edit` does. While
there is a plan to [migrate `toml` to be on
`toml_edit`](toml-rs/toml#340), it's not ready
yet and it's very easy to switch back to `toml` when it's ready.
@epage
Copy link
Member Author

epage commented Jan 13, 2023

Once #435 is merged, all of the blockers will be resolved and we can move forward with changing out tomls parser.

epage added a commit to epage/toml_edit that referenced this issue Jan 18, 2023
This will help with debugging

This only addresses toml-rs#396 for `toml_edit`.  The rest won't be addressed
until toml-rs#340 is resolved.
@epage
Copy link
Member Author

epage commented Jan 18, 2023

#457 switched de support to toml_edit, only leaving ser support.

epage added a commit to epage/toml_edit that referenced this issue Jan 19, 2023
This is the other main half of toml-rs#340.  Still have deprecations and tests left

Note that strings are rendered differently, see toml-rs#287

By extension this also finishes up toml-rs#396.

BREAKING CHANGES
- `impl Display for toml::Value` now renders as values, not documents, see instead `Table`
- `toml::ser::Serializer` only serializes documents, instead see `toml::ser::ValueSerializer`
- `toml::ser::tables_last` is removed, no longer needed
- `toml::ser::to_vec` is removed to mirror the loss of `from_slice`
- atm `toml::ser::to_string_pretty` just causes larger arrays to be indented
- `toml::ser::Error` is now opaque
- `toml::ser::Serializer::pretty_string` was removed
- `toml::ser::Serializer::pretty_string_literal` was removed
- `toml::ser::Serializer::pretty_array` was removed
- `toml::ser::Serializer::pretty_array_indent` was removed
- `toml::ser::Serializer::pretty_array_trailing_comma` was removed
- `toml::ser::Serializer` is now used used by value, rather than `&mut`

Fixes toml-rs#396
@epage
Copy link
Member Author

epage commented Jan 19, 2023

#470 takes care of ser support.

That just leaves

  • moving around tests
  • identifying serde issues this fixes and closing them out
  • marking deprecations in 0.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-serde Area: Serde integration C-enhancement Category: Raise on the bar on expectations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants