Add option to format with Unicode syntax. #206

kindaro · 2022-06-18T19:17:31Z

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.

github-actions · 2022-06-18T19:17:44Z

👋 @kindaro
Thank you for raising your pull request.
Please make sure you have followed our contributing guidelines. We will review it as soon as possible!

Reviewer: Please verify the following things have been done, if applicable.

CHANGELOG.md has been updated
Configuration docs in README.md have been updated
fourmolu.yaml updated to stay in sync with config in README.md
Tests have been added

brandonchinn178 · 2022-06-18T19:38:17Z

Thanks! Can you make the code and config a bit less opinionated? A simple bool for the config, and rename the args to whenUnicodeOtherwise...

georgefst · 2022-06-18T22:58:24Z

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.

kindaro · 2022-06-19T11:38:49Z

Like so?

brandonchinn178 · 2022-06-19T16:03:41Z

Perfect, thanks! Can you follow the instructions in CONTRIBUTING.md to update docs + tests for your new option?

src/Ormolu/Config.hs

kindaro · 2022-06-19T17:41:46Z

@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.

brandonchinn178 · 2022-06-19T18:22:37Z

Sorry, DEVELOPER.md

georgefst · 2022-06-19T19:07:14Z

I think this might actually work best as a ternary option, with the values always, never and detect. Where detect will output unicode if and only if the UnicodeSyntax extension is enabled, similarly to what we do with ImportQualifiedPost. I feel detect would be the sensible default.

georgefst · 2022-06-19T19:09:07Z

Is there any particular reason why this doesn't cover all unicode syntax e.g. forall/``∀`?

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.

kindaro · 2022-06-20T07:12:17Z

Change the metaHelp text.
Detect the UnicodeSyntax extension.
Add missing Unicode symbols.

kindaro · 2022-06-20T08:16:07Z

So, as George says, ImportQualifiedPost is automatic. And it does not have any command line flags to it. If the extension is enabled, the syntax is used — easy!

Why then have a command line option for UnicodeSyntax? Maybe we should hard-wire it in the same way as ImportQualifiedPost? You want it — you switch the extension on (you have to anyway, lest parse errors befall you). You want none of it — you switch the extension off.

brandonchinn178 · 2022-06-20T16:40:55Z

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.

georgefst · 2022-06-20T18:54:29Z

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 UnicodeSyntax is added to GHC202*?

And I agree that if we're going to go with the non-configurable version, then it's worth at least mentioning upstream.

brandonchinn178 · 2022-06-20T18:58:09Z

I see little harm in it being configurable.

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.

kindaro · 2022-06-23T10:56:49Z

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?

georgefst · 2022-06-23T10:59:35Z

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».

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.

brandonchinn178 · 2022-06-23T14:54:13Z

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

georgefst · 2022-06-23T22:42:40Z

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.

kindaro · 2022-06-25T16:56:10Z

It makes sense that fourmolu should behave exactly as ormolu with some «compatibility» configuration But ormolu declined to add this feature. Therefore, we must have at least a setting to switch this feature off.
The extension UnicodeSyntax can be supplied to fourmolu with a specialized option, which will result in this feature being switched on for all modules. Therefore, the option to switch this feature on is redundant.

I shall add settings «never» and «automatic». Hopefully soon.

kindaro · 2022-08-12T15:51:01Z

Another argument for giving a flag to switch the extension off: currently, if you enable ImportQualifiedPost, fourmolu will format all your imports that way — then, if you remove the extension, it could not parse the code anymore! So, there is no way back, even though in principle fourmolu can as easily format the imports back.

By extension, we may want to set the UnicodeSyntax extension on with an appropriate option to fourmolu and yet ask it to write ASCII syntax. So, we cannot say that the option «always» is not needed either. Enabling the extension is different from setting the option to «always».

kindaro · 2022-08-12T16:12:23Z

Hey I have been busy but finally — rebased, added a three way option, and automatic detection of the extension. Ready for review!

brandonchinn178

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

src/Ormolu/Config.hs

src/Ormolu/Printer/Combinators.hs

brandonchinn178 · 2022-08-12T23:16:33Z

Also, it looks like there are test failures. The biggest one being that fourmolu.yaml should be updated to include unicode: never (being that fourmolu.yaml represents the configuration to match ormolu's style)

kindaro · 2022-08-13T18:47:25Z

@brandonchinn178 Done!

brandonchinn178

Seems like there are still some test failures

brandonchinn178 · 2022-08-13T19:20:00Z

data/fourmolu/unicode-syntax/output-UnicodeAlways.hs

+    _ == _ = False
+
+add1 ∷ Quote m ⇒ Int → m Exp
+add1 x = [|x + 1⟧


should the opening quasiquote here be unicode?

Yes it should. I shall investigate.

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.

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.

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.

tests/Ormolu/Config/PrinterOptsSpec.hs

CHANGELOG.md

kindaro · 2022-08-13T19:40:17Z

Seems like there are still some test failures

Yes, I changed some white space in input.hs and forgot to refresh the output files because I thought it should not make difference but turns out it does.

It is the small things that will be my undoing.

(It does not do anything yet.)

kindaro · 2022-11-08T18:39:58Z

I rebased, there are no test failures, changelog.d updated, ready to ship.

tests/Ormolu/Config/PrinterOptsSpec.hs

brandonchinn178 · 2022-11-08T18:50:46Z

src/Ormolu/Printer/Combinators.hs

+    UnicodeDetect | unicodeExtensionIsEnabled -> unicodeText
+    UnicodeDetect | otherwise -> asciiText


Followup PR: this could be either

UnicodeDetect -> if unicodeExtensionIsEnabled then unicodeText else asciiText

or

UnicodeDetect | unicodeExtensionIsEnabled -> unicodeText | otherwise -> asciiText

Yes, if would be good.

brandonchinn178 · 2022-11-08T18:52:06Z

src/Ormolu/Printer/Combinators.hs

@@ -38,6 +38,7 @@ module Ormolu.Printer.Combinators
    breakpoint,
    breakpoint',
    getPrinterOpt,
+    whenUnicodeOtherwise,


This doesn't need to be exported anymore, right? Followup PR could keep this as an internal helper

brandonchinn178 · 2022-11-08T18:52:55Z

src/Ormolu/Printer/Combinators.hs

+closeQuote = "⟧" `whenUnicodeOtherwise` "|]"
+
+-- | Print @⊸@ or @%1 ->@ as appropriate.
+lolly = "⊸" `whenUnicodeOtherwise` "%1 ->"


Followup PR: let's rename this to linearArrow

Are you sure? This naming is not my idea. It is called lolly in GHC's source code.

oh interesting. Ok 🤷

brandonchinn178 · 2022-11-08T19:32:24Z

Thanks!

kindaro force-pushed the add-unicode-preference branch from b6d41bf to 938f118 Compare June 19, 2022 11:36

dpwiz reviewed Jun 19, 2022

View reviewed changes

src/Ormolu/Config.hs Outdated Show resolved Hide resolved

kindaro mentioned this pull request Jun 20, 2022

Write with Unicode syntax when UnicodeSyntax is seen to be switched on? tweag/ormolu#899

Closed

brandonchinn178 added the new config option Adds or would add new configuration option label Aug 5, 2022

kindaro force-pushed the add-unicode-preference branch from 3763321 to 1d6405f Compare August 12, 2022 16:11

kindaro requested review from georgefst and dpwiz August 12, 2022 16:12

brandonchinn178 reviewed Aug 12, 2022

View reviewed changes

src/Ormolu/Config.hs Outdated Show resolved Hide resolved

src/Ormolu/Printer/Combinators.hs Outdated Show resolved Hide resolved

kindaro force-pushed the add-unicode-preference branch 2 times, most recently from 4114918 to e3c7144 Compare August 13, 2022 19:06

brandonchinn178 reviewed Aug 13, 2022

View reviewed changes

kindaro force-pushed the add-unicode-preference branch from deb7e62 to 4c5726c Compare November 8, 2022 17:05

kindaro added 8 commits November 8, 2022 20:14

Add unicode command line option.

02a9299

(It does not do anything yet.)

Add a combinator for Unicode choice.

53e80f3

Add literal combinators for Unicode.

8880b34

Do wield Unicode literals.

d6d871e

fixup! Do wield Unicode literals.

a6165b6

Mention unicode in the default configuration.

4394eb1

Adjust test suite.

0a24b81

Update fluff.

0c6f31e

kindaro force-pushed the add-unicode-preference branch from 4c5726c to 0c6f31e Compare November 8, 2022 17:46

kindaro requested review from brandonchinn178, georgefst and dpwiz and removed request for georgefst, brandonchinn178 and dpwiz November 8, 2022 18:40

brandonchinn178 approved these changes Nov 8, 2022

View reviewed changes

brandonchinn178 merged commit 44389c1 into fourmolu:main Nov 8, 2022

kindaro mentioned this pull request Nov 8, 2022

Polish the unicode option. #249

Closed

4 tasks

		UnicodeDetect \| unicodeExtensionIsEnabled -> unicodeText
		UnicodeDetect \| otherwise -> asciiText

Add option to format with Unicode syntax. #206

Add option to format with Unicode syntax. #206

Conversation

kindaro commented Jun 18, 2022 • edited by brandonchinn178 Loading

github-actions bot commented Jun 18, 2022 • edited by brandonchinn178 Loading

brandonchinn178 commented Jun 18, 2022

georgefst commented Jun 18, 2022

kindaro commented Jun 19, 2022

brandonchinn178 commented Jun 19, 2022

kindaro commented Jun 19, 2022

brandonchinn178 commented Jun 19, 2022

georgefst commented Jun 19, 2022

georgefst commented Jun 19, 2022

kindaro commented Jun 20, 2022 • edited Loading

kindaro commented Jun 20, 2022 • edited Loading

brandonchinn178 commented Jun 20, 2022

georgefst commented Jun 20, 2022

brandonchinn178 commented Jun 20, 2022

kindaro commented Jun 23, 2022

georgefst commented Jun 23, 2022

brandonchinn178 commented Jun 23, 2022

georgefst commented Jun 23, 2022

kindaro commented Jun 25, 2022 • edited Loading

kindaro commented Aug 12, 2022 • edited Loading

kindaro commented Aug 12, 2022

brandonchinn178 left a comment

Choose a reason for hiding this comment

brandonchinn178 commented Aug 12, 2022 • edited Loading

kindaro commented Aug 13, 2022

brandonchinn178 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kindaro Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

kindaro commented Aug 13, 2022

kindaro commented Nov 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonchinn178 commented Nov 8, 2022

kindaro commented Jun 18, 2022 •

edited by brandonchinn178

Loading

github-actions bot commented Jun 18, 2022 •

edited by brandonchinn178

Loading

kindaro commented Jun 20, 2022 •

edited

Loading

kindaro commented Jun 20, 2022 •

edited

Loading

kindaro commented Jun 25, 2022 •

edited

Loading

kindaro commented Aug 12, 2022 •

edited

Loading

brandonchinn178 commented Aug 12, 2022 •

edited

Loading

kindaro Nov 8, 2022 •

edited

Loading