dhall-format: Detect character set from input #2108
Conversation
basile-henry
force-pushed
the
basile/dhall-format-detect
branch
2 times, most recently
from
8f0bf17 to
a3622e8
Compare
basile-henry
force-pushed
the
basile/dhall-format-detect
branch
2 times, most recently
from
f0da6f1 to
43a65dd
Compare
|
@basile-henry: Note that I'd like to hold off on merging this until we first cut a small 1.37.1 release to push out the fix in #2112 |
|
@Gabriel439 No worries, it can wait. I was also wondering if we should get feedback/approval from others in the dhall-lang/dhall-lang#1086 thread. Someone in that thread wanted to be able to configure the language server to use Unrelated: About cutting fixes for previous releases, do you know that Mergify can backports PR automatically? https://docs.mergify.io/commands.html#backport 😄 I think we just need to keep around a branch for the latest release (or however many releases we want to keep supporting) and then we can easily backport a PR to that branch. |
- Only Dhall syntax is considered for the detection (strings and comments don't affect the decision) - If no Unicode used in expression default to ASCII - Add switch to specify Unicode explictly
This makes Expr's Eq not depend on which CharacterSet is used
Remove dubious AlwaysEq, and instead remove CharacterSet as part of denote
basile-henry
force-pushed
the
basile/dhall-format-detect
branch
from
6edad2b to
fefd169
Compare
| data ServerConfig = ServerConfig | ||
| { asciiOnly :: Bool | ||
| -- ^ Use ASCII symbols rather than fancy unicode when formatting and linting | ||
| -- code. | ||
| { chosenCharacterSet :: Maybe CharacterSet | ||
| } deriving Show |
There was a problem hiding this comment.
@PanAeon Do you know if I need to modify something else to make this new config format work? Maybe somewhere in https://github.com/PanAeon/vscode-dhall-lsp-server ? I haven't seen anything that sets the LSP config there though.
There was a problem hiding this comment.
It looks like it works just fine without modifying the VS code extension, and this config can still be set from settings.json manually. I might try to make this new option accessible from the extension's UI, but it's not necessary for now.
Is that accurate? I am extremely in favour of ASCII mode so I'd love it. But I haven't seen discussion of it, and dhall has historically defaulted to unicode, so I don't want to get my hopes up ;) If it is accurate, are there eventual plans to drop unicode, or would it be supported as opt-in forever? |
Yes! The main way people use the Unicode syntax is by letting the formatter change their operators automatically (I don't think anyone types in Unicode, but maybe I'm wrong).
Yes, and that's one of the reasons I've been so slow at merging this PR. Ideally I'd like more feedback on this new way of handling character sets. I know some people have become attached to the Unicode character set and now they have one extra step to use it (either type a Unicode operator once or use
As far as I know there are no plans to drop Unicode. |
|
@basile-henry: I'm fine merging this now. There's no need to hold off merging any longer, in my opinion @timbertson: This comment explains why the default is Unicode if the file has a mixture of ASCII and unicode: dhall-lang/dhall-lang#1086 (comment). If the file is all ASCII then it will default to ASCII |
Alright, let's do it! We can always revert/fixup as needed in future PRs 👍 |
|
@basile-henry: Thank you for contributing this! 🙂 |
This PR implements the suggestion from dhall-lang/dhall-lang#1086
dhall formatcan now detect which character set is used in the input instead of defaulting to Unicode.