-
Notifications
You must be signed in to change notification settings - Fork 696
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
cabal exact-printer #7626
base: master
Are you sure you want to change the base?
cabal exact-printer #7626
Conversation
Should this go in the upcoming |
@@ -42,12 +44,14 @@ import Prelude () | |||
-- | A Cabal-like file consists of a series of fields (@foo: bar@) and sections (@library ...@). | |||
data Field ann | |||
= Field !(Name ann) [FieldLine ann] | |||
| Comment !ann !(CommentLine ann) |
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.
Why?
Comments can be extracted separately? See how cabal-fmt
does the job. (Also GHC AST is not polluted with comments, they are attached afterwards. That's why there is ann
in the Field
structure too).
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 was just something I wrote for quick prototyping. I'll checkout both cabal-fmt
and ghc-exactprint
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.
We should keep in mind that most time this code is hit when solver is traversing dozens if not hundreds of cabal files. The less stuff is done in hot path, the more responsive cabal-install
is.
The "interactive" usage should not make compromises to the primary function. I'd argue that if it becomes a friction then having two separate parsers is not impossible idea either.
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.
Hmm... it's something to ponder; while having a solid base to build on top of is great, having a clean slate to shape the feature to our current needs, while not affecting the older code, is very appealing.
@pranaysashank, likely, however it's best we wait (before moving anything) for #7620 to be merged first. |
#7620 has been merged – what’s the status of this PR? |
@ptkato: thanks for this version of the PR. Do you intend to work on it in the future? |
@Mikolaj I'd like to, but at the moment I can't tackle something this size. |
@ptkato: thanks. No pressure, just asking. I wonder if perhaps this can be split into smaller parts and tackled gradually by multiple people. |
Please include the following checklist in your PR:
This PR aims to centralise the efforts done toward our goal of coming up with a exact-printer for cabal.
Refer to #7544 for more detailed information and discussion.