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

Omit top level struct #6

Closed
ModProg opened this issue Apr 16, 2023 · 7 comments · Fixed by #10
Closed

Omit top level struct #6

ModProg opened this issue Apr 16, 2023 · 7 comments · Fixed by #10
Assignees

Comments

@ModProg
Copy link
Collaborator

ModProg commented Apr 16, 2023

This one I'm not sure about, but for a config file it feels quite nice:

  1. Remove redundant information (90% of config files would be a struct anyway)
  2. Remove unnecessary first indentation (granted, this could be achieved by making the default formatting:
{
a: {
    b: "hello"
}
}

but I'm not the biggest fan of that.)

Contra would be that this is a clear breakage to the rust like syntax, and it requires looking ahead till the first : to figure out, if the file root is a struct or a single value (thought the latter would be a single token AFAICT so probably not too bad to check).
Mainly the former is the reason though why I am a bit conflicted about this suggestion.

@ecton
Copy link
Member

ecton commented Apr 17, 2023

I actually was planning on adding this as an option. I think a "strict mode" shouldn't allow this, but that for configs, it makes a lot of sense.

@ecton ecton self-assigned this Apr 17, 2023
@ecton
Copy link
Member

ecton commented Apr 17, 2023

One thing I hadn't considered until taking a stab at this today was handling commas:

foo: 1,
bar: Baz

The commas are usually there to denote the separation of items in a list. Until now, Parser hasn't needed any end-of-line related information -- but if we want to get rid of the required commas on implicit maps, we'll want to be able to treat an end of line as an item delimiter, but only at the root level.

ecton added a commit to ecton/rsn that referenced this issue Apr 17, 2023
This commit adds the concept of an implicit map -- detecting where the
root starts with `Identifier Colon` and switching into map parsing.

I commented on khonsulabs#6 about the remaining "edge case" I hadn't thought of --
handling of commas vs new lines in this mode. Currently, commas are
still required, and whitespace is insignificant, but this likely will be
updated to support removing commas between key-value pairs as long as
there's an end-of-line.
@ModProg
Copy link
Collaborator Author

ModProg commented Apr 17, 2023

makes sense, it does feel weird to require , on the plain root struct. Though this raises an interesting question concerning multi token values e.g. path::path in case we support that (there could be a line break between path:: and path) or Ident {...} where the { could start on the next line.

Due to the requirnments it would still be unambiguous, but not exactly easy to read:

a: b
{
}
{
}: a 
{
}:a
{
}

@ecton
Copy link
Member

ecton commented Apr 18, 2023

I've thought about this a bit more, and I'm pretty sure that if you're using Serde with this implicit map feature, there will be no ambiguities if we simply allow omitting commas in implicit map parsing mode, and force implicit map keys to always be single identifiers.

By forcing the keys to always follow a ident : pattern, we remove a lot of ambiguity, because on the value side of the colon, there's no patterns where an ident is trailing or where two idents can be used consecutively.

ecton added a commit to ecton/rsn that referenced this issue Apr 18, 2023
This commit adds the concept of an implicit map -- detecting where the
root starts with `Identifier Colon` and switching into map parsing.

I commented on khonsulabs#6 about the remaining "edge case" I hadn't thought of --
handling of commas vs new lines in this mode. Currently, commas are
still required, and whitespace is insignificant, but this likely will be
updated to support removing commas between key-value pairs as long as
there's an end-of-line.
@ModProg
Copy link
Collaborator Author

ModProg commented Apr 18, 2023

That is true, we only have the ambiguity for the struct like patterns (actually rust has a similar one).

But yeah, this would be solved by only allowing scalars and idents as LHS.

@ModProg
Copy link
Collaborator Author

ModProg commented Apr 18, 2023

Maybe allowing arbitrary keys wouldn't be too bad, from a pure parsing perspective, but I don't know how useful they are either.

let map;
let key = None;
loop {
  if tokens.peek().is_ident() {
    let path = tokens.parse_path()?;
    if tokens.peek().is_start_of_struct_like() {
      let struct = tokens.parse_struct_like()?;
      if tokens.peek().is_colon() {
        map.insert(key, ident);
        key = Some(struct);
      } else {
        map.insert(key, NamedStructLike(ident, struct);
        key = None;
      }
    } else {
      map.insert(key, ident);
      key = None;
    }
  } else {
    map.insert(key, tokens.parse_value()?);
    let key = None;
  }
}

@ecton
Copy link
Member

ecton commented Apr 18, 2023

I think the key with this PR is that implicit maps are now their own thing -- we can define its key rules however we want. For now, it's ultra-restrictive and matches exactly what we'd expect from Serde-compatible serialization of a root-level named struct.

@ecton ecton closed this as completed in #10 Apr 18, 2023
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 a pull request may close this issue.

2 participants