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

Remove trailing commas in hjson files #552

Open
cousteaulecommandant opened this issue Jul 12, 2024 · 1 comment
Open

Remove trailing commas in hjson files #552

cousteaulecommandant opened this issue Jul 12, 2024 · 1 comment

Comments

@cousteaulecommandant
Copy link
Contributor

cousteaulecommandant commented Jul 12, 2024

Hjson files should not contain commas at the end of each line. In particular, they MUST NOT in the case of "quoteless strings". However, the use of these commas is widespread in X-HEEP, which results in incorrect Hjson files.

Quoting the Hjson documentation:

Quoteless Strings

[...]
Do not add commas or comments as they would become part of the string.

Furthermore (although less importantly):

Commas

[...]
You should omit optional commas [other than for quoteless strings] to make your data more readable.

{
  one: 1
  two: 2
}

However, there are plenty of Hjson files in X-HEEP that use trailing commas, which in many cases are illegal since they will be included as part of the string. The most common cases are "barewords" and hex values starting with 0x (which are actually strings, not numbers):

active:  low,
address: 0x10000000,

But I would just remove all the trailing commas, since they're entirely optional (when they're allowed) and the standard itself recommends removing them. This includes trailing commas after a ], or a },.

This issue seems to have been "fixed" in some places of the Python code by explicitly adding .strip(','), but this is a workaround that "fixes the symptom" rather than fixing the broken file. This can be problematic in a future; I recommend fixing all the .hjson files so that they're compliant with Hjson in this regard.

cousteaulecommandant added a commit to cousteaulecommandant/x-heep that referenced this issue Jul 12, 2024
Some hjson files in X-HEEP use trailing comma after a "quoteless string"
which isn't valid in the standard (https://hjson.github.io/syntax.html):

> Do not add commas or comments as they would become part of the string.

This includes "barewords" such as `active: low,` but also (surprisingly)
hexadecimal values such as `address: 0x10000000,` which is technically
also interpreted as a string and not an integer value.
(`42,`, `true,`, `false,` and `"string",` are OK though.)

This fixes issue esl-epfl#552.
It also deprecates certain workarounds in the code that were using
`.split(',')` or `.strip(',')` to address this issue (but the issue was
really that the file format was wrong, not that the parser failed).
cousteaulecommandant added a commit to cousteaulecommandant/x-heep that referenced this issue Jul 13, 2024
Some hjson files in X-HEEP use trailing comma after a "quoteless string"
which isn't valid in the standard (https://hjson.github.io/syntax.html):

> Do not add commas or comments as they would become part of the string.

This includes "barewords" such as `active: low,` but also (surprisingly)
hexadecimal values such as `address: 0x10000000,` which is technically
also interpreted as a string and not an integer value.
(`42,`, `true,`, `false,` and `"string",` are OK though.)

This fixes issue esl-epfl#552.
It also deprecates certain workarounds in the code that were using
`.split(',')` or `.strip(',')` to address this issue (but the issue was
really that the file format was wrong, not that the parser failed).
cousteaulecommandant added a commit to cousteaulecommandant/x-heep that referenced this issue Jul 13, 2024
Some hjson files in X-HEEP use trailing comma after a "quoteless string"
which isn't valid in the standard (https://hjson.github.io/syntax.html):

> Do not add commas or comments as they would become part of the string.

This includes "barewords" such as `active: low,` but also (surprisingly)
hexadecimal values such as `address: 0x10000000,` which is technically
also interpreted as a string and not an integer value.
(`42,`, `true,`, `false,` and `"string",` are OK though.)

This fixes issue esl-epfl#552.
It also deprecates certain workarounds in the code that were using
`.split(',')` or `.strip(',')` to address this issue (but the issue was
really that the file format was wrong, not that the parser failed).
cousteaulecommandant added a commit to cousteaulecommandant/x-heep that referenced this issue Jul 13, 2024
Some hjson files in X-HEEP use trailing comma after a "quoteless string"
which isn't valid in the standard (https://hjson.github.io/syntax.html):

> Do not add commas or comments as they would become part of the string.

This includes "barewords" such as `active: low,` but also (surprisingly)
hexadecimal values such as `address: 0x10000000,` which is technically
also interpreted as a string and not an integer value.
(`42,`, `true,`, `false,` and `"string",` are OK though.)

This fixes issue esl-epfl#552.
It also deprecates certain workarounds in the code that were using
`.split(',')` or `.strip(',')` to address this issue (but the issue was
really that the file format was wrong, not that the parser failed).
@cousteaulecommandant
Copy link
Contributor Author

I have created PR #553 to fix the affected hjson files, removing the commas that confuse the hjson parser (but leaving out other commas, although the standard recommends removing those as well).
I have tried to be careful with the latest commit at main which also touched some of the affected hjson files (including a CI/CD-related file).

cousteaulecommandant added a commit to cousteaulecommandant/x-heep that referenced this issue Jul 20, 2024
Some hjson files in X-HEEP use trailing comma after a "quoteless string"
which isn't valid in the standard (https://hjson.github.io/syntax.html):

> Do not add commas or comments as they would become part of the string.

This includes "barewords" such as `active: low,` but also (surprisingly)
hexadecimal values such as `address: 0x10000000,` which is technically
also interpreted as a string and not an integer value.
(`42,`, `true,`, `false,` and `"string",` are OK though.)

This fixes issue esl-epfl#552.
It also deprecates certain workarounds in the code that were using
`.split(',')` or `.strip(',')` to address this issue (but the issue was
really that the file format was wrong, not that the parser failed).
cousteaulecommandant added a commit to cousteaulecommandant/x-heep that referenced this issue Jul 20, 2024
The Hjson format specification (https://hjson.github.io/syntax.html)
also discourages the use of trailing commas at the end of a line,
which are harmless (valid) but superfluous and confusing:

> You should omit optional commas to make your data more readable.

It has been decided that adopting this style will be less confusing
and avoid possible repetitions of issue esl-epfl#552 in the future.

This commit removes all the trailing commas from hjson files,
after a meticulous process to verify that none of these trailing commas
had a specific meaning; such as in this hypothetical scenario:
```
    description: '''This is the description's first line,
                    which has a trailing comma.'''
```
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

No branches or pull requests

1 participant