-
Notifications
You must be signed in to change notification settings - Fork 45
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
Improve CDDL syntax #870
base: main
Are you sure you want to change the base?
Improve CDDL syntax #870
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in CDDL ; (semicolon) indicates a start of the comment and they should not affect parsing (https://datatracker.ietf.org/doc/rfc8610/#:~:text=normalization%20processes.%0A%0A%20%20%20Example%3A-,%3B%20This%20is%20a%20comment,-person%20%3D%20%7B%20g%20%7D%0A%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20g)
@@ -1695,7 +1695,7 @@ session.PacProxyConfiguration = ( | |||
) | |||
|
|||
session.SystemProxyConfiguration = ( | |||
proxyType: "system" | |||
proxyType: "system", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commas are actually optional and the parser should handle that, but it is good to include them anyway as per https://datatracker.ietf.org/doc/rfc8610/#:~:text=The%0A%20%20%20%20%20%20comma%20is%20actually%20optional%20(not%20just%20in%20the%20final%20entry)%2C%20but%20it%0A%20%20%20%20%20%20is%20considered%20good%20style%20to%20set%20it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OrKoN Would you suggest adding commas on the last item as well (e.g. behind Extensible
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, this would at least help with future changes along those lines, ensuring that only the added or removed item is affected, rather than modifying an existing one just to add or remove a comma.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we then keep trailing //
for the same reason?
@@ -1660,7 +1660,7 @@ session.ProxyConfiguration = { | |||
session.DirectProxyConfiguration // | |||
session.ManualProxyConfiguration // | |||
session.PacProxyConfiguration // | |||
session.SystemProxyConfiguration // | |||
session.SystemProxyConfiguration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, trailing //
are also valid per the CDDL grammar (and I don't see any text in the RFC that forbidd them), so if you keep trailing commas, you could keep //
for the same reason.
Triggers christian-bromann/cddl#107.
Preview | Diff