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

Add parens around pretty printing of application #102

Merged

Conversation

mattpolzin
Copy link
Collaborator

@mattpolzin mattpolzin commented Oct 7, 2023

It's been really fun to explore Dhall via Idris interop so thanks for putting this library together!

I found that pretty printing almost created valid Dhall if I had e.g. an Optional List of Naturals, but it lacked parentheses. This PR over-applies parentheses so that the result is always valid Dhall even if sometimes it has more parentheses than needed.

before:

Optional List Text

after:

Optional (List Text)

@alexhumphreys
Copy link
Owner

Thanks for all the changes! I'm a bit out of the loop on idris at the moment unfortunately (got a reminder to check once a week for issues/PRs here), but if you're planning on making more changes I'd be happy to add you as a collaborator. As long as the tests are passing you could merge your own PRs 👍

Speaking of, the tests are failing on this one, but I think it's just a matter of adding parenthesis on line 5, 13, 14, 15 here. After that it should be good to go

@mattpolzin
Copy link
Collaborator Author

@alexhumphreys If you want to give me merge privvies I won’t argue! I do try very hard not to abuse such things, so I wouldn’t merge anything that fell outside of “better support the dhall spec” without waiting for your input anyway.

@alexhumphreys
Copy link
Owner

Sounds good, just invited you there 🙌

@mattpolzin mattpolzin force-pushed the parenthesize-pretty-print-app branch from b337583 to 8c202e1 Compare October 13, 2023 04:17
@mattpolzin mattpolzin merged commit d552273 into alexhumphreys:main Oct 13, 2023
@mattpolzin mattpolzin deleted the parenthesize-pretty-print-app branch October 13, 2023 14:18
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

Successfully merging this pull request may close these issues.

2 participants