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

Dotted keys are not supported #91

Closed
5 of 6 tasks
Tracked by #58
andrewhickman opened this issue Sep 3, 2020 · 11 comments
Closed
5 of 6 tasks
Tracked by #58

Dotted keys are not supported #91

andrewhickman opened this issue Sep 3, 2020 · 11 comments
Assignees
Labels
C-enhancement Category: Raise on the bar on expectations

Comments

@andrewhickman
Copy link

andrewhickman commented Sep 3, 2020

Status


The following TOML file is not parsed successfully:

ssh.public-key-path = "C:/Users/andrew.hickman/.ssh/id_rsa.pub"

It fails with

TOML parse error at line 68, column 4
   |
68 | ssh.public-key-path = "C:/Users/andrew.hickman/.ssh/id_rsa.pub"
   |    ^
Unexpected `.`
Expected `=`

However the toml crate parses this correctly, and looking at the ABNF grammar I think it is a valid TOML file

toml = expression *( newline expression )

expression =/ ws keyval ws [ comment ]

dotted-key = simple-key 1*( dot-sep simple-key )
@andrewhickman
Copy link
Author

Just spotted #73 and tried using that branch instead, and I can confirm that it parses this successfully

@ordian
Copy link
Member

ordian commented Sep 3, 2020

Support for toml 0.5 is tracked in #58.

@ghost ghost mentioned this issue Jan 10, 2021
@epage
Copy link
Member

epage commented Aug 24, 2021

Some initial notes when researching this

Table-types summary

Tables

  • Empty super tables are optional
  • You cannot use a table to overwrite an item (value or table)
  • Tables can be out-of-order

Dotted Keys

  • Implicit tables can only merge with a previous implicit table from dotted key
  • Table can be used to create child-table

Inline Tables

  • Immutable

How toml_edit currently does it

Table

  • Order-preserving map of Key to Item
  • Create implicit tables for empty super tables

InlineTable

  • Order-preserving map of Key to Item

Proposed API in #73

Dotted keys are just an Item to serve as a pointer to the appropriate table

  • The table gets a has_dotted marker (unsure which implicit wasn't used)

Concerns

  • Directly exposing the dotted-key pointer creates undefined behavior for users directly manipulating it

How tomlkit does it

Root Container appears to directly represent the document as-is

  • A mapping is maintained between the direct representation and the users mental model
  • Proxy types are used when the direct representation needs adjusting for a smooth API

Table and InlineTable

  • independent Containers for their key-value pairs
  • Implicit super tables are created for each level of a dotted key

Not quite sure how the rest of this is handled.

@epage
Copy link
Member

epage commented Aug 24, 2021

The idea I'm playing with and trying to vet was briefly mentioned in #122, of having

pub struct Table {
    ...
    format: TableFormat,
}

pub enum TableFormat {
    ImplicitTable,
    DottedTable,
    InlineTable(...),
    Table(...),
}
  • To retain dotted key ordering, we will add a position: Option<usize> to items
    • Changing a table's TableFormat will clear positions of all values (retaining insertion order) so that we don't end up with weird ordering when a dotted key turns into full Tables. and new items are inserted (problem case: first item would have position N and next would have position 0)
  • Formatting information will be wrapped in a Option
    • Detect when formatting is undefined due to table-type change (ie changing types by users will make it None
    • Also allow detecting when users did not set any formatting
    • This will allow formatters to decide how they should format undefined formatting, whether naively or doing something fancier

TODO Finish filing this out

  • What will Item and Value look like to support the above

@epage epage mentioned this issue Aug 26, 2021
3 tasks
@epage epage added the C-enhancement Category: Raise on the bar on expectations label Aug 26, 2021
@epage epage self-assigned this Aug 26, 2021
@epage
Copy link
Member

epage commented Sep 1, 2021

Some updates

  • I was looking at merging table types
    • A TableFormat enum was being added along with structs for its invariants, like StandardTable and InlineTable
    • A table type could be left as unknown until formatting
    • Problems:
      • Led to complications when doing different operations that require knowing what the table type is, like sorting. Maybe that didn't have to be? I could just sort any ways
  • I was looking into making the parser an iterator over Expression
    • Removes all the callbacks
    • Reduced the cosntant lookups because I kept the in-work table around
    • Problem: By not erroring inside of a parser, but outside, we lose automatically getting the position in the error message
    • Could still reduce the lookups by applying the same technique
    • Could still use expressions and have a shared callback

Overall, at this point, I am looking at keeping InlineTable and Table separate. There will be a way to mark either table as Dotted. I'll then add position information to TableKeyValuePari so we dotted positioning works like global table positioning.

I'm thinking of rolling out support gradually. First, we'll just parse the dotted tables without preserving their style and maybe not handling all error cases correctly. We'll then slowly add in those features.

@ordian
Copy link
Member

ordian commented Sep 1, 2021

Thanks for digging into this.

Something slightly related: you're probably familiar with #63 and table positioning. I think we should make to_string_in_original_order to_string, making the former the default Display behavior.

@epage
Copy link
Member

epage commented Sep 1, 2021

I was thinking of that! Glad we are on the same page.

I've also wondered if we should switch away from formatting with Display. I think its behavior can surprise people (e.g. value.display() including decor when they probably expect the underlying value) and the interface makes things a bit limited (can't pass around state without ...Display types)

@epage
Copy link
Member

epage commented Sep 1, 2021

(and yes, global table position was inspiring my table key positioning idea so we don't need markers)

@ordian
Copy link
Member

ordian commented Sep 1, 2021

I've also wondered if we should switch away from formatting with Display. I think its behavior can surprise people (e.g. value.display() including decor when they probably expect the underlying value) and the interface makes things a bit limited (can't pass around state without ...Display types)

Sure, we should only use Display when it makes sense. I think it does for the whole Document, but maybe not so for other types as you've mentioned.

@epage epage changed the title Top-level dotted keys are not supported Dotted keys are not supported Sep 2, 2021
epage added a commit to epage/toml_edit that referenced this issue Sep 2, 2021
epage added a commit that referenced this issue Sep 2, 2021
epage added a commit to epage/toml_edit that referenced this issue Sep 2, 2021
epage added a commit that referenced this issue Sep 2, 2021
@epage
Copy link
Member

epage commented Sep 2, 2021

Random implementation notes from order preserving

  • I initially tried storing the order in Key but it felt more natural in the parser to put it in the TableKeyValue
  • I switched to TableKeyValue but we lose that information after we pass through get_children. So its back to being in the key

@epage
Copy link
Member

epage commented Sep 3, 2021

I'm splitting #163 out and closing this

@epage epage closed this as completed Sep 3, 2021
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

3 participants