-
Notifications
You must be signed in to change notification settings - Fork 148
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
Switch parser to toml_document or tomllib #69
Comments
Wow, that looks pretty impressive! Thanks for letting me know. Do you plan on writing a PR for this? Otherwise I'll probably get around to writing one myself in the few days. |
well, it’s not on cargo yet and is probably not feature-complete yet (vosen/toml_document#1) so there’ll probably be some holdups if we don’t want to maintain a fork until everything we need is in there |
Right. I was thinking more of a WIP PR, to see how far this would currently work, not one we'd merge immediately. This could potentially also drive the design of toml_document itself. |
sure! but without vosen/toml_document#1, we can’t get anywhere 😄 |
that is done now, we can revisit this! |
In #15, https://github.com/joelself/tomllib is discussed. Can someone look into the difference between it and https://github.com/vosen/toml_document/ ? |
well, neither have open bugs, but tomllib seems better documented |
Yes, but sadly, the documentation (Readme) currently includes:
|
haha well, that’s kinda important |
I think we should use toml_document, this crate has everything we need. |
Could be. I can't really tell because this crate has basically no documentation and even though there are tests, they are written in quite weird way 😅 Also, it has not been updated for a year, which also makes me hesitant. |
So... I've written yet another toml parser for fun - toml_edit (reminds me of this xkcd). It's not published yet, partly due to the current API being uber ugly and inconsistent. The biggest blocker for using it in Contributions are welcome :) |
Nice, @ordian! I saw your gitter link and wrote down a few things while reading some of the code |
This seems like the most significant barrier to more widespread cargo-edit usage at the moment, so it'd be nice to get this moving. I think we've got three options for the new toml parser:
Arguably, toml_document is likely to be the best technical solution, given that it claims to provide pretty good guarantees of not altering the toml document unnecessarily. However, given that toml_document is completely undocumented, and seemingly no longer under active development, integration is liable to be excessively painful. Thus, it looks as if toml_edit may be the best choice. Though this is contingent on toml_edit getting a 0.1 release. @ordian - do you think you'll be able to make the API changes you want to soon? Otherwise, I'm quite tempted to start integrating against the current API. |
@bjgill thanks for writing this. I want to clarify on guarantees provided by [dependencies.nom] # comment 1
version = "3"
[dependencies] # comment 2
hello = "1" will be replaced with [dependencies] # comment 2
hello = "1"
[dependencies.nom] # comment 1
version = "3" (Also array of tables is grouped in one place) Actually this constraints were only enforced in the latest pr in order to make
Well, I can try to write an abstraction around inline and non-inline tables this weekend-ish. |
Hey, Molten is already fully style, WS, comment, and order preserving in tests, modulo:
After that it needs error management and a proper API. I'm committed to finishing and maintaining it so it should be a good way to go for cargo-edit. |
181: Start world domination r=killercup a=ordian cc #15, #69. This is an experiment with [toml_edit](https://github.com/ordian/toml_edit) to figure out what the right API (LeopoldArkham/Molten#8, cc @LeopoldArkham) is for the upcoming integration with Molten, but also can be useful by itself while Molten is not yet ready. Basically, all we need is an `Item`, which can represent anything (`None`, `Table`, `InlineTable`, `String`, ...), `IndexMut<&str, Output=Item>` implementation for Item and a bunch of downcasting methods (`is_table`, `as_table`, ...). Deleting an item is just replacing it with `None`.
We've switched to toml-edit, so this is no longer relevant. |
This allows us to fix #15
The text was updated successfully, but these errors were encountered: