Skip to content
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

Merge vscode-dotty-syntax in this repo to get Scala 3 indentation to work nicely #179

Open
smarter opened this issue Dec 21, 2020 · 15 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@smarter
Copy link
Member

smarter commented Dec 21, 2020

https://github.com/smarter/vscode-dotty-syntax/ does two things:

  • Install a completion provider for every keyword so that vscode doesn't auto-complete a keyword into some other word when pressing Enter after typing a keyword.
  • Set a custom indentationRules so that vscode auto-indents a newline after a keyword, and auto-unindent after end tokens (this one is maybe more likely to be annoying and could be dropped). Note that this one won't work if it is added in this extension right now when metals-vscode is also used because metals-vscode will override it (https://github.com/scalameta/metals-vscode/blob/acbb383e0ee606da45aeb56803de0f82b1cb4a1b/src/extension.ts#L925), so we should also integrate the indentation rules from metals in this extension and then remove them from metals-vscode
@tgodzik
Copy link
Contributor

tgodzik commented Jan 20, 2022

I've added some simpler rules for indentation here: https://github.com/scalameta/metals-vscode/blob/main/src/extension.ts#L1181-L1196

We shouldn't use the indentation rules from https://github.com/smarter/vscode-dotty-syntax/ as there is no way to figure out deindent. We could migrate what Metals does into vscode-scala-syntax if there is anyone that feels like it would be useful.

@smarter
Copy link
Member Author

smarter commented Jan 20, 2022

as there is no way to figure out deindent.

Is that a problem? To deindent, users press backspace.

@tgodzik
Copy link
Contributor

tgodzik commented Jan 20, 2022

as there is no way to figure out deindent.

Is that a problem? To deindent, users press backspace.

Yes, because VS Code will automatically indent to the latest indentation if you do enter, which means the users will have to do backspace a lot, which is not a perfect UX.

For example if you have:

def a = 
  def b = 
    def c=
      1
    b
  c
end a

if you press enter before end a it will get indented, which is not expected.

@smarter
Copy link
Member Author

smarter commented Jan 20, 2022

ah I see what you mean thank you, so what happens with your version if you press enter at various points?

@tgodzik
Copy link
Contributor

tgodzik commented Jan 20, 2022

ah I see what you mean thank you, so what happens with your version if you press enter at various points?

It will remain the same as the previous indentation, but increase after pressing enter after the specific keywords.

@smarter
Copy link
Member Author

smarter commented Jan 20, 2022

OK, that sounds fine, so we should move all that logic from metals-vscode into this repo as discussed in scalameta/metals-vscode#512. vscode-scala-syntax also does this:

Install a completion provider for every keyword so that vscode doesn't auto-complete a keyword into some other word when pressing Enter after typing a keyword.

which still looks like it's required and will also need to be copied in this repo.

@tgodzik
Copy link
Contributor

tgodzik commented Jan 20, 2022

Install a completion provider for every keyword so that vscode doesn't auto-complete a keyword into some other word when pressing Enter after typing a keyword.

Should we indeed add it? vscode syntax doesn't know what Scala version you have, while Metals can provide keyword completions already for Scala 2. We will need to add them for Scala 3 for sure, but that is coming.

@smarter
Copy link
Member Author

smarter commented Jan 20, 2022

Editing Scala 3 code with just vscode-scala-syntax should be pleasant to use, but right now it isn't because if I do:

val thenable = 1
if foo then[ENTER]

vscode will auto-complete to thenable. IMO this isn't acceptable, and having a few extra keywords available as completion in a Scala 2 project is a price worth paying to avoid this issue. More generally, I think official Scala tooling should commit to providing the best user experience for Scala 3 and not treat it as a second class citizen, otherwise users will never switch and we'll be stuck in migration hell indefinitely.

@smarter
Copy link
Member Author

smarter commented Jan 20, 2022

(And we can't rely on fixing this in metals since web versions of vscode like the one you get on github by pressing '.' on a repository can't use vscode-metals but can use this extension)

@tgodzik
Copy link
Contributor

tgodzik commented Jan 21, 2022

So what will happen when you do:

val other = the[ENTER]

?

It would only autocomplete then, but not thenable ? I don't believe it's a problem to write most of the keywords by hand and o still allow people have names that may or may not include keywords. Also, what about soft keywords?

More generally, I think official Scala tooling should commit to providing the best user experience for Scala 3 and not treat it as a second class citizen, otherwise users will never switch and we'll be stuck in migration hell indefinitely.

On the other hand, we can't start treating Scala 2 as a second class citizen.

(And we can't rely on fixing this in metals since web versions of vscode like the one you get on github by pressing '.' on a repository can't use vscode-metals but can use this extension)

So this should be a bit more complex logic than just hardcoding keywords, we should take into account the context.

@smarter
Copy link
Member Author

smarter commented Jan 21, 2022

It would only autocomplete then, but not thenable ?

Have you tried it? If the is a valid completion it won't autocomplete anything, otherwise it will pick the first completion it finds which might be then.

I don't believe it's a problem to write most of the keywords by hand

I agree, the only reason to have this keyword completion logic is to workaround the weird behavior of vscode of autocomplete-on-enter, maybe alternatively we could turn off that behavior in vscode-scala-syntax, all I care about is that users writing Scala 3 code don't need to fight their editor.

we can't start treating Scala 2 as a second class citizen.

All I'm saying is we shouldn't make Scala 3 worse because of Scala 2, Scala 2 will be legacy eventually so this would be counter-productive.

@robmwalsh
Copy link

I've added some simpler rules for indentation here: https://github.com/scalameta/metals-vscode/blob/main/src/extension.ts#L1181-L1196

We shouldn't use the indentation rules from https://github.com/smarter/vscode-dotty-syntax/ as there is no way to figure out deindent. We could migrate what Metals does into vscode-scala-syntax if there is anyone that feels like it would be useful.

@fommil and I are both working on scala LSPs now, so moving this out of metals would be useful. There's probably also a broader conversation to be had about getting metals, ensime-tng, and whatever I'm doing to play nicely together. Perhaps something like with BSP support where there's an option to use Bloop or SBT as the build server? VSCode doesn't really let you manage conflicts between extensions. To try out Ensime I had to manually disable metals, install the Ensime .VSIX and restart vscode. Going back was more difficult - I had disable ensime, then actually uninstall and reinstall metals. Simply re-enabling the extension didn't work for some reason. I didn't try too hard to figure out why since I needed a functional IDE and ensime wasn't handling the repo I was working on at the time (still early days for ensime).

Ideally I'd like see a comment pallet option "choose LSP", then when I know I need to save as much juice as possible I can easily switch to Ensime, when I want a more fully featured experience I can swap back to metals. I'm aiming for somewhere in between, but with more focus on staying as useful as possible when code isn't compiling.

@tanishiking
Copy link
Member

tanishiking commented Oct 25, 2022

moving this out of metals would be useful.

👍 Actually, there's an issue in metals-vscode scalameta/metals-vscode#512 but requires some work.


There's probably also a broader conversation to be had about getting metals, ensime-tng...

The rest part sounds more like providing a nice way to switch metals and ensime, which should be discussed in another issue.

I'm not familiar with the ensime vscode extension well, but the idea off the top of my head would be merging the metals-vscode and ensime's vscode extension into one extension and letting users switch (that I'm not sure it's a good idea).
AFAIK, there's no way to control other extensions from one extension other than setting Recommended extensions.
The first thing we should do would be to check why disabling one extension doesn't work, I guess.

@robmwalsh
Copy link

Thanks - I'll create an issue in vscode-metals. I don't think merging them is a good idea but I have an idea which I'll mention in the new issue.

@tgodzik
Copy link
Contributor

tgodzik commented Oct 25, 2022

I think moving some of the rules out of Metals is a good idea, but I haven't had the time to do it. Especially, that it was mostly needed for Metals and we could do more experimentation there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants