-
Notifications
You must be signed in to change notification settings - Fork 481
[Textual] Parse constants with parens #7391
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
base: master
Are you sure you want to change the base?
[Textual] Parse constants with parens #7391
Conversation
|
I'm not sure this is better. I would rather have an inconsistent syntax with a single way of doing things, than a partially consistent syntax with non-canonical ways of doing things. |
|
I don't care either way. My justification is that for constants we mostly reuse Haskell syntax and there parsing of redundant parens is mostly supported: If any of the reviewers don't like this PR, feel free to close it. |
|
|
||
| -- | Parser for constants of the given type. | ||
| constantOf :: ExpectParens -> DefaultUni (Esc a) -> Parser a | ||
| constantOf expectParens uni = |
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 think the expectParens parameter is useless now?
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.
Nope, we still can't allow (con data I 42), it has to be (con data (I 42)).
While (con (list data) [I 42]) is fine.
So the distinction still exists.
kwxm
left a comment
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'm not too sure about the arbitrarily nested parentheses.
Also, we should probably have some conformance tests here.
| constantOf :: ExpectParens -> DefaultUni (Esc a) -> Parser a | ||
| constantOf expectParens uni = | ||
| case uni of | ||
| try (trailingWhitespace . inParens $ constantOf ExpectParensNo uni) <|> case uni of |
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.
This now accepts
(con (list data) [ (((((((((((I 5))))))))))), B #FF ])
and
(con unit (((((((((((((((((((()))))))))))))))))))))
Do we really want that?
| constantOf :: ExpectParens -> DefaultUni (Esc a) -> Parser a | ||
| constantOf expectParens uni = | ||
| case uni of | ||
| try (trailingWhitespace . inParens $ constantOf ExpectParensNo uni) <|> case uni of |
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.
Also, the prettyprinter still prints list elements without parentheses. I suppose we have to choose one way or the other, so that seems fine.
This makes the parse recognize constants with extra parens in them as requested in #7383.
I don't feel like we want to make any of those extra parens mandatory, but there's no harm in being lenient and parsing them.