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

Enforce constraints #19

Closed
ordian opened this issue Aug 5, 2017 · 2 comments
Closed

Enforce constraints #19

ordian opened this issue Aug 5, 2017 · 2 comments
Assignees
Labels
C-enhancement Category: Raise on the bar on expectations

Comments

@ordian
Copy link
Member

ordian commented Aug 5, 2017

Problems

  1. Currently, toml spec does not have any positional constraints on tables (cf toml/issues/446). E.g. this is a valid toml:
[[bin]] # bin 1
[a.b.c.d]
[a]
[other.table]
[a.b.c.e]
[[bin]] # bin 2
[a.b.c]
[[bin]] # bin 3

This greatly complicates internal design of toml_edit, e.g. we have to store this order somewhere outside of the tables (document), and in order to support table insertion and deletion on Table, we have to store a pointer to the document in it. That's why we don't allow a user to own a Table, providing only a (mut) ref to a Table.

If we are free to reorder the tables when printing them, we can simplify the design and get rid of the no-ownership constraint. This, in turn, would allow us to have a single Value enum and implement serde_json-like nice API and get rid of unsafe code!

  1. Another annoying thing is whitespaces in headers.
[ 'this  '   . is . a .valid.    "header..." ]

In order to preserve this layout, we have to store the whole header in the Table, but then Table assignment would be unfortunate, because header will also be mutated, while in hashmap, storing subtable index, there still be the old key.

Proposal

  1. Allow any order when parsing the document, but internally store all children after theirs parent. Also store array of tables in one place. When printing the toml from above, it would look like:
[[bin]] # bin 1
[[bin]] # bin 2
[[bin]] # bin 3
[a]
[a.b.c]
[a.b.c.e]
[a.b.c.d]
[other.table]

Unresolved: should we sort the children as well?

  1. Drop all whitespaces in the header.
['this  '.is.a.valid."header..."]

cc @killercup, what do you think?

@ordian ordian added the C-enhancement Category: Raise on the bar on expectations label Aug 5, 2017
@ordian ordian added this to the publish 0.1 milestone Aug 5, 2017
@ordian ordian self-assigned this Aug 5, 2017
@killercup
Copy link

I'm totally fine with this being a… let's call it "intention-preserving pretty printer" :)

At the last rust.cologne meetup, we talked about a more simplistic approach to a toml reader/writer specifically for cargo-edit. One design we landed on was Having the whole toml AST consist of slices of the input string (maybe represented as rope). Adding/removing an entry would then "just" be inserting another slice at some position, or removing slice(s). I doubt this scales well to a more general purpose library, though.

@ordian ordian removed this from the publish 0.1 milestone Aug 5, 2017
@ordian
Copy link
Member Author

ordian commented Aug 5, 2017

Thanks for the feedback!

I think I've got an idea on how to improve API without implementing this proposal. As for duplication in Table and InlineTable, a viable approach worth investigating may be to create a common trait TableLike and implement a method on them, which will return a trait object.

I'm going to close this issue.

@ordian ordian closed this as completed Aug 5, 2017
ordian added a commit that referenced this issue Aug 28, 2017
ordian added a commit that referenced this issue Aug 28, 2017
bors bot added a commit that referenced this issue Aug 28, 2017
29: refactor(all): implement proposal #19 r=ordian a=ordian
epage added a commit to epage/toml_edit that referenced this issue Apr 26, 2024
chore(ci): Ensure CI job always runs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Raise on the bar on expectations
Projects
None yet
Development

No branches or pull requests

2 participants