Skip to content

Move Brittany plugin from HIE #66

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

Merged
merged 4 commits into from
Apr 19, 2020
Merged

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Apr 17, 2020

Trivial translation, need to re-use existing parsing result instead of parsing it brittany again.

  • Implement Tests
  • Add AGPL flag
  • Prefer direct cradle over stack for executing tests

@fendor fendor requested a review from alanz April 17, 2020 15:22
Copy link
Collaborator

@alanz alanz left a comment

Choose a reason for hiding this comment

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

The mechanics look good.

The tests need to be updated though, there are some original ones that I brought over that have been commented out.

Also, very important, we need to put a flag around it so it can be disabled by people who do not want to have to depend on AGPL code.

There are plenty of examples out there, e.g. https://hackage.haskell.org/package/ghc-exactprint

@fendor
Copy link
Collaborator Author

fendor commented Apr 18, 2020

Tests need to be refactored and fixed, but they are there already

@fendor
Copy link
Collaborator Author

fendor commented Apr 18, 2020

Also, ditches stack as our primary test runner.

Copy link
Collaborator

@alanz alanz left a comment

Choose a reason for hiding this comment

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

This looks good, thanks.

There does not seem to be a CI report for it?

And your summary indicates it is not done yet?

@fendor fendor requested a review from alanz April 19, 2020 12:42
@fendor
Copy link
Collaborator Author

fendor commented Apr 19, 2020

some of the summary can not easily be adressed yet.
We can not move brittany code to use pPrintModule, since we can not obtain the Anns parameter which we get from ghc-exactprint, afaict, and we do not have this parsing artefact.
Moreover, the given API can not be used for formatting only a selection.
However, I would like to adress that in follow-up PRs, including providing a patch to brittany to enable range formatting.

Copy link
Collaborator

@alanz alanz left a comment

Choose a reason for hiding this comment

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

I am happy to merge this as use, it is a straight migration of the existing brittany plugin. Best to do one thing only in a PR.

Changes to the API to re-use the existing ParsedSource is a separate task. And is likely to require changes to ghcide to enable generation of API Annotations.

@alanz alanz merged commit cfe4369 into haskell:master Apr 19, 2020
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