Skip to content

add basic_toml_conf feature #112

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

JeromeSchmied
Copy link
Contributor

@JeromeSchmied JeromeSchmied commented Mar 27, 2025

add feature to migrate from the massive toml dependency to it's light-weight hard fork (now unmaintained, but functional) basic-toml. AFAIK it supports most toml syntax, except for date-time

@JeromeSchmied
Copy link
Contributor Author

maybe as non-default feature?

@JeromeSchmied
Copy link
Contributor Author

does this look good, or should I fork?

@JeromeSchmied JeromeSchmied marked this pull request as ready for review March 27, 2025 08:25
@JeromeSchmied JeromeSchmied changed the title misc(deps): migrate from toml to basic-toml add basic_toml_conf feature Mar 27, 2025
Copy link
Contributor

@deg4uss3r deg4uss3r left a comment

Choose a reason for hiding this comment

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

Overall I would be okay adding this in, do you have any metrics for what this move would save the average developer (compile times, binary size, etc)?

I don't think I have ever reached for basic_toml over toml before. I can also talk to @epage to see if toml-rs would be interested in taking in basic_toml since it was very recently unmaintained.

@epage
Copy link

epage commented Mar 28, 2025

In abstracting the use of basic-toml from the user, you should probably make it clear that

  • Its unmaintained
  • It is non-conformant

Keep in mind dtolnay created basic-toml to reduce the build times of trybuild and then dropped it in dtolnay/trybuild#239, in part because of dtolnay/trybuild#257

With those caveats, I would personally think it questioable to support but then again, I maintain the name of the crate it was forked from (it was forked from toml 0.5 and toml 0.6 was a complete rewrite).

I don't think I have ever reached for basic_toml over toml before. I can also talk to @epage to see if toml-rs would be interested in taking in basic_toml since it was very recently unmaintained.

I have no interest in maintaining it. Part of my condition for taking on maintaining toml was that I wouldn't be maintaining multiple TOML parsers but could consolidate them.

I am playing with a new parser implementation for toml_edit that may reduce some of the overhead (including possibly being usable outside of toml_edit and directly by toml). This is all an experiment that is very much a low priority (usually while I'm traveling to conferences).

@JeromeSchmied
Copy link
Contributor Author

JeromeSchmied commented Mar 29, 2025

thanks for the info, I understand this basic_toml is definitely not usable in many cases, but IMO and use-case being so simple amd lightweight is highly admirable, even in it’s current state, not evolving anymore - of course only if one knows the trade-offs as they’re properly documented - especially when someone chooses this crate over the quite complete config-rs

as for the refactors and docs, I’ll look into them next week as they’re reasonable

@JeromeSchmied JeromeSchmied requested a review from deg4uss3r March 29, 2025 10:09
@JeromeSchmied JeromeSchmied marked this pull request as draft March 29, 2025 10:11
@JeromeSchmied JeromeSchmied marked this pull request as ready for review April 4, 2025 17:31
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.

3 participants