Add fn/end and do/end as brackets in language configuration

[ollien]

Fixes #83

Adds fn/end and do/end as brackets in language configuration. This enables bracket colorization, and enables other bracket-related features (e.g. "Go to Bracket").

Screenshot:

[Blond11516]

Hi @ollien. Thanks a lot for taking the time to open a PR!

I'll admit I'm a little on the fence about this. I'm not entierly opposed to this change, but I would instinctively expect keywords to always be the same color. I'm also slightly worried that fn, do and end might be considered as brackets in some unexpected scenarios, though I can't think of any off the top of my head.

Looking at the corresponding issue in vscode-elixir-ls it looks like Lukasz has run into and fixed some issues so I'll have to take a better look at how he did it.

I'll run this by the rest of the Lexical team and get back to you as soon as possible!

[ollien]

Totally makes sense. One note, though

I'm not entierly opposed to this change, but I would instinctively expect keywords to always be the same color

FWIW, the colorization still has to be enabled within the VSCode settings. You could always disable this feature for your editor as a whole, or for elixir specifically. Having the option is nice, though, IMO.

[zachallaun]

Looking at the corresponding issue in vscode-elixir-ls it looks like Lukasz has run into and fixed some issues so I'll have to take a better look at how he did it.

Definitely agreed with this @Blond11516. Something to do with unbalancedBracketScopes in the syntax, it looks like.

You could always disable this feature for your editor as a whole, or for elixir specifically. Having the option is nice, though, IMO.

While I personally dislike colorized do/end, I definitely do want colorized brackets. What I think we may want to document is that users can override the colorized brackets in their settings if they wish. I haven't tested it with this PR specifically, but if I add this to my VS Code settings.json, it correctly stops colorizing everything except for parens:

  "[elixir]": {
    "editor.language.colorizedBracketPairs": [
      ["(", ")"]
    ]
  }

So, I presume that people could revert to the old colorizing behavior while still having bracket matching (which is nice), by using:

  "[elixir]": {
    "editor.language.colorizedBracketPairs": [
      ["(", ")"],
      ["[", "]"],
      ["{", "}"]
    ]
  }

[zachallaun]

That's all to say, if we can make sure that this doesn't run into the same issue that ElixirLS did initially, I'm for it, and perhaps we can document that colorized do/end and fn/end can be removed by overriding editor.language.colorizedBracketPairs in the README.

[ollien]

Thanks guys. I've used this configuration for some time and am indeed running into some edge cases. I'll take a look soon about what can be done

[Blond11516]

@zachallaun Thanks for the added context, I looked a bit for a way to disable specific bracket pairs but couldn't find it after a quick search!

Documenting this would probably be good enough, but ideally I'd like to do this through a proper configuration option, which I feel would be more idiomatic. With that said I doubt there's a way to configure colorized bracket pairs programatically, so docs might have to be it.

One last thing @ollien, if you could update the changelog that would be great! Given this is the first change since the last release, you can add an ## [Unreleased] section and add the change there.

[Blond11516]

From what I can quickly find there seems to be two ways to go about controlling this programatically:

1. Update the workspace configuration to change the editor.language.colorizedBracketPairs setting. This would mean binding two settings together and keeping them in sync (or handling the cases when they inevitably will not be) feels like a rat's nest I don't want to get into. This would also run the risk of overriding user-made custom changes to the config, which would be really bad.
2. Update the language configuration to remove fn/end and do/end from the list of brackets. This removes the coloring, but it also means we lose bracket pair highlighting and the ability to use features like "Select to bracket" which would be unfortunate.

Neither of these sound very appealing to me, so I think we're simply going to document it.

@ollien I'm thinking of adding a ## Configuration > ### Manual configuration section to the README, like so:

## Configuration

### lexical.server.releasePathOverride

...

### lexical.notifyOnServerAutoUpdate

...

### Manual configuration

Would you be willing to add that if it looks like a clear and easily discoverable place to you?

[ollien]

Sorry for the delay here, I've just found the chance to work on this. While my preliminary testing shows great improvement (thank you for linking that ElixirLS issue!), I'm going to run this for a couple of days to see if any other major issues come up.

[Blond11516]

Great! Seeing as this is basically the same configuration as Elixir LS I'm pretty confident it should work well.

[Blond11516]

Feel free to merge this when you're confident there aren't any obvious issues left.

[zachallaun]

(Also note that, if there is an issue after merging, it'll be okay 🙂)

[ollien]

Been using this since Monday and had no complaints. Merge away!