Skip to content

Conversation

@jhrcek
Copy link
Contributor

@jhrcek jhrcek commented Sep 25, 2019

Description of the change

Here's the initial attempt to implement support for --no-comments switch (fix #417)

The most future-proof approach seems to be to parse the dhall template file, remove comments from AST and pretty-print it before writing the file. This however is currently not possible due to dhall stripping most comments while parsing (see dhall-lang/dhall-haskell#145).

This PR could be considered a hack taking advantage of this Dhall behavior and simply does parse + pretty print of dhall templates, which "accidentally" strips comments. This impl. will potentially break in the future when Dhall parser preserves comments, but I added tests to catch this.

I thought of couple of alternative approaches, but I find them even more hacky/brittle than the current PR:

  1. Do something like Text.dropWhile (/="-}") on the dhall template files (would break once more than 1 comment is present in the template or comment is somewhere else than in the beginning)
  2. Have another "no comment" templates and copy those instead (would require maintenance of duplicate templates)

Checklist:

  • Added the change to the "Unreleased" section of the changelog
  • Added some example of the new feature to the README
  • Added a test for the contribution (if applicable)

P.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 😊

Copy link
Member

@f-f f-f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhrcek sorry for taking a while to review. This looks great!
I only left minor comments, but we're almost there 🙂

@jhrcek
Copy link
Contributor Author

jhrcek commented Oct 2, 2019

Thanks for the feedback! spago works great and is really useful. Also the source code is nicely readable. I'd like to contribute some more to practice my Haskell :-) Let me know if there are some issues that you'd consider helpful to fix, otherwise I'll pick something with "good first issue" label

@f-f
Copy link
Member

f-f commented Oct 2, 2019

@jhrcek thanks for the kind words, I'm happy to hear this 😊

I just went through all the issues marked with "good first issue", and they should all be fine to pick up! I also tried to provide context and/or an idea for implementation in there, but if there are any details missing or confusing I'd be happy to detail more in the relevant threads 🙂

Copy link
Member

@f-f f-f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks for doing this! 👏

@f-f f-f merged commit 6887b85 into purescript:master Oct 2, 2019
@jhrcek jhrcek deleted the noComments branch October 4, 2019 12:21
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.

Feature request: Add a flag to spago init that excludes comments package.dhall and spago.dhall

2 participants