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

Skipping brackets on the top level structure #297

Closed
halvnykterist opened this issue Feb 2, 2021 · 14 comments
Closed

Skipping brackets on the top level structure #297

halvnykterist opened this issue Feb 2, 2021 · 14 comments

Comments

@halvnykterist
Copy link

halvnykterist commented Feb 2, 2021

Would it be possible/reasonable to skip the surrounding (), {}, or [] on the top level structure in document?
As it is a config needs to be saved as something like:

(
    player_name: "abc",
    foo: 5,
    bar: [
        1,
        5,
        22,
    ],
)

It'd be a lot cleaner to my mind if we could skip the surrounding brackets and just write it as:

player_name: "abc",
foo: 5,
bar: [
    1,
    5,
    22,
],
@kvark
Copy link
Collaborator

kvark commented Feb 2, 2021

On one hand, I'm worried that this would be yet another complication.
On the other hand, it depends on how difficult this is to implement as a format extension. Maybe not that hard?
Finally, it would help wgpu traces a bit. We write entries as they come, and often end up with a file that doesn't have a closing bracket. Following the suggested syntax would make the problem go away.

@madwareru
Copy link

madwareru commented Feb 2, 2021

This would be a big pain to support in my Intellij IDEA plugin. Please don't do this.

When you omit () or {} it becomes a problem to make parser understand what it sees

Anyway, I don't see a problem to write it like this if you don't want to waste lines of code:

(player_name: "abc",
foo: 5,
bar: [
    1,
    5,
    22,
],)

@zakarumych
Copy link

@madwareru there are three possibilities. And content in () cannot be parsed the same way as content in {} or []. They overlap only if empty.

@madwareru
Copy link

I don't argue that there is a possibility to support it. I argue about the complexity it leads to. Anyway, if it would only be an extension, I'd may just say that my plugin does not support it, so may be this is not such a problem

@madwareru
Copy link

madwareru commented Feb 2, 2021

content in () cannot be parsed the same way as content in {} or []

in fact, content in () and [] could be parsed the same way (a tuple vs a list), but author didn't suggest [] omittance

@halvnykterist
Copy link
Author

halvnykterist commented Feb 2, 2021

in fact, content in () and [] could be parsed the same way (a tuple vs a list), but author didn't suggest [] omittance

Or, indeed, as a list vs a fixed length array. I'd definitely want to omit [] same as the others (that's largely what my usecase is), I'll edit the first post to reflect this.
Now that I think about it, some of this same parsing ambiguity should exist there:

(
    foo: "bar",
    baz: "qux",
)

and

(
    ( foo: "bar" ),
    ( foo: "baz" ),
)

are both possible, you need to check whether something is a tuple/fixed array or struct by checking the contents.

@zakarumych
Copy link

What purpose in using () for tuples? Tuple is essentially a fixed sized array of heterogeneous types.

@kvark
Copy link
Collaborator

kvark commented Feb 2, 2021

We use () for all heterogeneously-typed constructors, everywhere.

@github-actions
Copy link

Issue has had no activity in the last 180 days and is going to be closed in 7 days if no further activity occurs

@github-actions github-actions bot added the stale label Nov 18, 2021
@kvark kvark reopened this Dec 3, 2021
@kvark
Copy link
Collaborator

kvark commented Dec 17, 2021

comment

@kvark kvark reopened this Dec 17, 2021
@github-actions github-actions bot removed the stale label Dec 18, 2021
@juntyr
Copy link
Member

juntyr commented Dec 30, 2021

I've given this some thought. If we deserialize with type information, then adding the extension isn't a problem. However, things get tricky with deserialize_any. I think it would be very counterintuitive to have an extension that can break RON parsing (all the other extensions merely change parsing but you still have a valid RON document, leaving out brackets makes it invalid as there is now lots of ambiguity that we cannot resolve without type information). One way around that would be to have one extension per bracket type ((, [, and {), preferably mutually exclusive, which mean that the brackets have been left out (I.e. they are now always reinserted during parsing). This would resolve the ambiguity issues, but wouldn't be very ergonomic. Maybe a better route would be to add this as a separate option in the Options config, communicating that this is a format breaking option that must be mirrored in ser and de. There it would also be much easier to enforce that only one kind of bracket can be implied. Thoughts @torkleyy?

@torkleyy
Copy link
Contributor

@MomoLangenstein I don't think we should support this, it completely changes the grammar of RON. Right now extensions do not interfere with parsing (only deserialization, that is the data model) and I don't think the parser should be configurable, because that would complicate everything. RON would no longer be a format but rather a "format kit" if that makes sense.

@juntyr
Copy link
Member

juntyr commented Jan 14, 2022

@torkleyy That makes sense. Shall we close this issue then?

@torkleyy
Copy link
Contributor

Yeah.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants