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
Allow trailing delimiters #956
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.
I would be fine with removing support for {, = }
but I don't have strong opinions either way. That was an oversight from my attempt to standardize leading commas and not something I carefully thought through
Doesn’t allowing a trailing delimiter require backtracking all the way in parsers? I implemented that for the nix parser some time ago, but we scrapped it because of that. |
@Profpatsch: I believe it's still doable without full backtracking. Using commas as an example, after parsing any given comma-separated element you parse one of: try ("," *> "})
<|> "}"
<|> ("," *> parseElement *> parseRest) ... which limits backtracking to a limited number of characters |
Isn't that lookahead rather than backtracking then ? I don't remember that being a performance sink |
Does anyone object to this then ? |
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.
Can we have tests for the case of having both a leading and a trailing delimiter? eg [ , 3 , ]
is valid according to the new ABNF.
@Nadrieril: I still think this proposal is a good idea |
I think this is ready to be merged |
Yeah sorry I haven't had the bandwidth for dhall recently, and I wanted to add the tests that @philandstuff suggested |
b8e302d
to
8dfcdcb
Compare
8dfcdcb
to
ec8db97
Compare
... as standardized in dhall-lang/dhall-lang#956
... as standardized in dhall-lang/dhall-lang#956
Fixes #512.
I discovered that the grammar allowed
{ , = }
for an empty record literal. I'm not sure if we want to keep that.