-
Notifications
You must be signed in to change notification settings - Fork 62
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 option to format with Unicode syntax. #206
Conversation
👋 @kindaro Reviewer: Please verify the following things have been done, if applicable.
|
Thanks! Can you make the code and config a bit less opinionated? A simple bool for the config, and rename the args to whenUnicodeOtherwise... |
This is cool, and I'm interested in trying it on some of my smaller personal projects. But I agree with @brandonchinn178 that more neutral language would be preferable, seeing as the current wording doesn't necessarily reflect all maintainers' views on unicode syntax. |
b6d41bf
to
938f118
Compare
Like so? |
Perfect, thanks! Can you follow the instructions in CONTRIBUTING.md to update docs + tests for your new option? |
@brandonchinn178 There is no such file in this repository. Can you link me to it? Better even, tell me what you want me to do. |
Sorry, DEVELOPER.md |
I think this might actually work best as a ternary option, with the values |
Is there any particular reason why this doesn't cover all unicode syntax e.g. I don't mind if the constructs currently implemented are the only ones you care about! But if so, we should open a follow-up issue tracking the fact that we should implement the rest. |
|
So, as George says, Why then have a command line option for |
That makes sense to me! Have you checked to see if this is something Ormolu wants? If so, it would be better to make the change upstream instead. |
I see little harm in it being configurable. But I'm not that bothered either way. If someone has turned on the extension, it does seem odd for them not to want unicode output. Then again, what if And I agree that if we're going to go with the non-configurable version, then it's worth at least mentioning upstream. |
That's fair. I think this is something Ormolu would be interested in anyway, so I think we should wait and see how that conversation pans out. If they don't want it, we can merge it here. If they do, we could think about making an option to forcibly turn it on. |
They do not want it, so we are back here. I say there should only be two options: «automatic» and «never». If you try to add Unicode syntax to a module where the extension is not enabled, there will be a parse error, so it is pointless to add an option «always». Agreeable? |
My concern was that a user's environment might be set up such that Fourmolu doesn't know the extension is active. But in that case, I guess they can just pass the extension to Fourmolu explicitly. |
Personally, I dont think we need any option. If someone wants to disable it for a particular module, they can turn off the extension with NoUnicodeSyntax |
I have a weak preference towards the ternary option, but I don't want to bikeshed. I'm happy with no option if that's what others prefer. We can always add options later if someone has a use case. |
I shall add settings «never» and «automatic». Hopefully soon. |
Another argument for giving a flag to switch the extension off: currently, if you enable By extension, we may want to set the |
3763321
to
1d6405f
Compare
Hey I have been busy but finally — rebased, added a three way option, and automatic detection of the extension. Ready for review! |
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.
Looks great, thanks! Please make sure to do everything in this list: https://github.com/fourmolu/fourmolu/blob/main/DEVELOPER.md#adding-a-new-configuration-option
Also, it looks like there are test failures. The biggest one being that |
@brandonchinn178 Done! |
4114918
to
e3c7144
Compare
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.
Seems like there are still some test failures
_ == _ = False | ||
|
||
add1 ∷ Quote m ⇒ Int → m Exp | ||
add1 x = [|x + 1⟧ |
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.
should the opening quasiquote here be unicode?
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.
Yes it should. I shall investigate.
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.
For some reason, when I make the left bracket into Unicode, idempotence breaks. It will take some time to get to the bottom of this.
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.
The problem is in Ormolu as well — the parser parses ⟦
as [e|
and then we must print it as [e|
as well. Issue to Ormolu: tweag/ormolu#934.
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.
Ormolu fixed their problem. Once it it merged to fourmolu
, I can tweak the code so that ⟦x⟧
is handled correctly. Ideally I should like to have this pull request merged first — I had enough of rebasing it over and over.
Yes, I changed some white space in It is the small things that will be my undoing. |
deb7e62
to
4c5726c
Compare
(It does not do anything yet.)
4c5726c
to
0c6f31e
Compare
I rebased, there are no test failures, |
UnicodeDetect | unicodeExtensionIsEnabled -> unicodeText | ||
UnicodeDetect | otherwise -> asciiText |
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.
Followup PR: this could be either
UnicodeDetect -> if unicodeExtensionIsEnabled then unicodeText else asciiText
or
UnicodeDetect
| unicodeExtensionIsEnabled -> unicodeText
| otherwise -> asciiText
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.
Yes, if
would be good.
@@ -38,6 +38,7 @@ module Ormolu.Printer.Combinators | |||
breakpoint, | |||
breakpoint', | |||
getPrinterOpt, | |||
whenUnicodeOtherwise, |
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 doesn't need to be exported anymore, right? Followup PR could keep this as an internal helper
closeQuote = "⟧" `whenUnicodeOtherwise` "|]" | ||
|
||
-- | Print @⊸@ or @%1 ->@ as appropriate. | ||
lolly = "⊸" `whenUnicodeOtherwise` "%1 ->" |
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.
Followup PR: let's rename this to linearArrow
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.
Are you sure? This naming is not my idea. It is called lolly
in GHC's source code.
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.
oh interesting. Ok 🤷
Thanks! |
Resolves #134
Unicode looks delicious (with an appropriate font), but there is no input method for it out of the box. Therefore, most people avoid it. Now we do not need an input method — it will input itself, solving the problem for good.