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

[TOML] Add Syntax #4168

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

deathaxe
Copy link
Collaborator

@deathaxe deathaxe commented Feb 22, 2025

Resolves #4167

This PR adds new TOML default package.

Syntax takes some inspiration from https://packagecontrol.io/packages/TOML but has finally been re-written from scratch using specs from https://toml.io/en/

Reasons:

  1. version 2 syntax definition

  2. be less strict with regards to incomplete statements to avoid overeager illegal highlighting.
    It is arguable if current state of illegal value highlighting is already too much.

  3. design syntax primarily using named contexts with possible inheritance in mind

  4. this implementation is about 30% faster (in its initial state)

Remarks:

Both, tables (aka. sections) and keys can consist of fully qualified identifiers specifying a path of hash table keys to final value.

[foo . "bar" . 'baz']
buuz = "value"

is the same as

[foo]
bar . "baz" . 'buuz' = "value"

Both represent following JSON:

{
    "foo": {
        "bar": {
            "baz": {
                "buuz": "value"
            }
        }
    }
}

As discussed in #4167 it probably doesn't make sense to map section name and key name scopes to those of JSON representation as it would cause both being scoped meta.mapping.key string, which results in unexpected highlighting compared to GitConfig and INI syntax.

As keys can consist of multiple elements, it is questionable whether meta.mapping.key string convention is the appropriate choise at all, because using different scoping schemas for sections and keys doesn't feel right and scoping individual elements string.unquoted or string.quoted separated by period, doesn't add any value, too.

Hence this PR starts with scoping whole...

  1. [section.name] => meta.section entity.name.section (known from INI and GitConfig)
  2. key.name => meta.mapping.key entity.name.key (which was used earlier)

Only quotation punctuation and . spearator is scoped on top of those entity scopes.

@jrappen
Copy link
Contributor

jrappen commented Feb 22, 2025

Nice, appreciate the quick follow-up PR.

Copy link
Collaborator

@FichteFoll FichteFoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only wanted to take a "quick" glance while filling a time gap and did not review the tests.

datetime:
# https://toml.io/en/v1.0.0#offset-date-time
# https://datatracker.ietf.org/doc/html/rfc3339
- match: '{{date_time}}'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the date_time variable intended to be re-used or extendable? I generally try to avoid defining capturing groups inside variable and then refering to them in captures because it involves a lot of indirection, especially so with nested variable definitions, and is hard to debug. This also makes the variable hardly extendable because the number of capturing groups and their scope assigments are pre-defined by the base syntax.

I can see that the date and time segments are re-used as separate patterns below, so we could make an exception for those, but I also don't really think the reusability of these small variables is worth the confusion.

To illustrate, there is a large capturing group inside date_time that contains the entire time part, including separators, but without mental gymnastics I cannot tell which scope this corresponds to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

date_time probably used once, only, but various capture groups are defined in underlying time and date as well, which is used multiple times.

The only other way would be to fully drop those 3 vars and build those patterns individually in corresponding - match: statements, which would add up a bit of duplication, which was argued against in other situations in the past various times.

deathaxe added 9 commits February 24, 2025 14:18
It is popping, so singular
Comments may start at any point without whitespace in front.
This commit...

1. applies `meta.section` to whole line, including leading and trailing
   whitespace so possibly applied background color covers full width of
   a view, which provides better UX.

2. applies `meta.brackets` to `[...]` itself to introduce a scope, which
   only covers that part.
Follow syntax specification naming.
Create corresponding table-value and inline-table-value contexts.
To follow official specs, this commit ensures to terminate inline tables at eol.
deathaxe added 2 commits February 25, 2025 15:50
Make the keys green as values and the world complicated beyond necessary
by introducing dedicated string contexts for section headers, key names
and values so than each one can be scoped and treated individually.
@deathaxe
Copy link
Collaborator Author

deathaxe commented Feb 25, 2025

As some insist, let's make the keys green like strings...

grafik

Note: Current implementation has TOML (Jinja) in mind:

grafik

@michaelblyons michaelblyons added the T: feature major update for a syntax to support new language features label Feb 25, 2025
@FichteFoll
Copy link
Collaborator

FichteFoll commented Feb 26, 2025

As some insist

Did others also voice their opinions? So far I've only seen myself advocating for meta.mapping.key string and I would have reached out to some people via Discord, most likely, to collect others' opinions since these are fundamental questions with two different opinions so far.

I assume the color scheme of the screenshots is Mariana, i.e. ST default?

Edit: Can we in the current implementation already strip comments at the beginnings of lines, e.g. for Python inline script metadata?

@deathaxe
Copy link
Collaborator Author

Yeah, Mariana. The highlighting every new user gets without (being aware) of any customization being needed or required for more fency highlighting ;-)

By stripping comments you mean something like ... ?

grafik

@deathaxe
Copy link
Collaborator Author

deathaxe commented Feb 26, 2025

So far I've only seen myself advocating

Single oppinions have often been enough to act as veto.

Just followed it, also with regards to #4171

Even though Git Config already suffers the "string" in section overlay.

And with regards to mapping interpretation and attempt to create common scopes, this oppinion is justified and correct. I actually also started with them.

@FichteFoll
Copy link
Collaborator

FichteFoll commented Feb 28, 2025

By stripping comments you mean something like ... ?

Exactly. Looks good.

Yeah, Mariana. The highlighting every new user gets without (being aware) of any customization being needed or required for more fency highlighting

We really need a way to update Mariana ourselves or finally get SHQ to update it. The meta.mapping.key thing has been in use for JSON for 6 years now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
significant T: feature major update for a syntax to support new language features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add TOML syntax
4 participants