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

Preserve comments in lexer #5439

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@pikajude
Copy link

pikajude commented Jul 17, 2018

Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

For my tool stylish-cabal, I'd like to be able to preserve existing comments in a .cabal file.

This change passes all of the parser-related tests. Comments are retained up until the stuff in Distribution.FieldGrammar, where they get filtered out by takeFields.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Jul 17, 2018

/cc @phadej

@pikajude pikajude force-pushed the pikajude:lex-comment branch 2 times, most recently from 0de1ae2 to a2ba03f Jul 17, 2018

@pikajude

This comment has been minimized.

Copy link
Author

pikajude commented Jul 20, 2018

Latest commit has some pretty major changes in the structure of the cabal file parser, namely that comments are only parsed before elements now - otherwise in something like this:

flag foo
  manual: True

-- a test-suite
test-suite bar
  main-is: bar.hs

the "a test-suite" comment will be parsed as part of the manual field value (since comments ignore indentation).

This has the side effect of changing one of the error messages in parser-tests (specifically this one), which I'm leaving alone for now because I'd like input as to whether it's an acceptable change and if not, how I could go about fixing it.

@23Skidoo 23Skidoo requested a review from phadej Jul 24, 2018

@phadej

This comment has been minimized.

Copy link
Collaborator

phadej commented Jul 24, 2018

@phadej

This comment has been minimized.

Copy link
Collaborator

phadej commented Jul 24, 2018

@pikajude

This comment has been minimized.

Copy link
Author

pikajude commented Jul 25, 2018

@phadej you're absolutely right here, they could just be extracted alongside the other lexer steps without complicating everything. That's an approach I didn't think of since I've never tried to make a big change to a lexer before. Will push a commit later today using your suggestion.

@pikajude pikajude force-pushed the pikajude:lex-comment branch from 7859fae to 036c917 Jul 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.