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

Make completions work on overr<COMPLETE> and def<COMPLETE> #640

Merged
merged 1 commit into from Apr 10, 2019

Conversation

4 participants
@tgodzik
Copy link
Collaborator

commented Apr 5, 2019

Fixes #631. Previously, we needed to write def to autocomplete an override, now we can write a start of the 'override' word or a combination of letters in the def/val name and it will trigger the proper completion. When starting with override word it will add it to method even if overriding abstract methods.

Also fixes a bug with not autocompleting when overriding vals from traits.

@tgodzik tgodzik force-pushed the tgodzik:add-override-completions branch 5 times, most recently from 332a708 to 412d2f3 Apr 5, 2019

@olafurpg
Copy link
Member

left a comment

Nice job! A few comments, I'm still not entirely sure what the ideal behavior should be 🤔

@gabro gabro changed the title Make complications work on overr<COMPLETE> and def<COMPLETE> Make completions work on overr<COMPLETE> and def<COMPLETE> Apr 7, 2019

@gabro

This comment has been minimized.

Copy link
Collaborator

commented Apr 7, 2019

Tiny thing: I've renamed the original issue to use the "Fixes #" syntax so that GitHub automatically links this PR to the issue.

@tgodzik tgodzik force-pushed the tgodzik:add-override-completions branch 3 times, most recently from aae07a5 to 5236e71 Apr 9, 2019

@olafurpg
Copy link
Member

left a comment

Looking fantastic, only a few minor comments otherwise this PR is almost ready to go!

@tgodzik tgodzik force-pushed the tgodzik:add-override-completions branch 6 times, most recently from c961929 to 7dba383 Apr 9, 2019

Make complications work on overr<COMPLETE> and def<COMPLETE>.
Fixes #631. Previously, we needed to write def <COMPLETE> to autocomplete an override, now we can write a start of the 'override' word or a combination of letters in the def/val name and it will trigger the proper completion. When starting with override word it will add it to method even if overriding abstract methods.

Also fixes a bug with not autocompleting when overriding vals form traits.

@tgodzik tgodzik force-pushed the tgodzik:add-override-completions branch from 7dba383 to 670be1a Apr 10, 2019

@olafurpg
Copy link
Member

left a comment

LGTM 👍 Great work @tgodzik!

@olafurpg olafurpg merged commit bb60af8 into scalameta:master Apr 10, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
scalameta.metals Build #20190410.3 succeeded
Details
@jvican

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2019

I've been trying out this feature, it's looking slick. Here's a situation where I think it could be improved:

Screen Shot 2019-04-10 at 3 09 27 PM

I would expect the methods to be overridden to be at the top of the completion list. We rarely write expressions inside templates, we usually want to implement some methods, so I think the completion list should reflect that.

In this case, there are almost no identifiers in scope, but if there were to be more i would miss completely the completions on the methods to override...

@tgodzik tgodzik deleted the tgodzik:add-override-completions branch Apr 10, 2019

@olafurpg

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@jvican I don't feel super strongly about this so I'm open to change it. You can also make arguments in favor of keeping the override def completions at the bottom because

  • you can type more characters such as "over" or "overdef" or "ovdef" to filter out non-override def completions, whereas there are only 7 characters you can type to reach reference symbols like okhttp3
  • the sortText we return is quickly ignored by vscode anyways as you type more characters, see microsoft/vscode#71660

olafurpg added a commit to olafurpg/metals that referenced this pull request Apr 10, 2019

Welcome Marek and Tomasz to the team!
As part of a new collaboration between the Scala Center and VirtusLab,
Marek and Tomasz will be contributing to Metals development for the
coming months. They have already contributed several impressive pull
requests:

- `textDocument/foldingRange` (scalameta#632): code folding that understands Scala syntax.
- `textDocument/documentHighlight` (scalameta#621): highlight occurrences of a symbol in
  the current file.
- `textDocument/completion` (scalameta#640): override def completions from
  without the need to type "override def ".

It is my pleaseure to welcome them to the team and I look forward to see
what more comes out of this collaboration :)

olafurpg added a commit to olafurpg/metals that referenced this pull request Apr 10, 2019

Welcome Marek and Tomasz to the team!
As part of a new collaboration between the Scala Center and VirtusLab,
Marek and Tomasz will be contributing to Metals development for the
coming months. They have already contributed several impressive pull
requests:

- `textDocument/foldingRange` (scalameta#632): code folding that understands Scala syntax.
- `textDocument/documentHighlight` (scalameta#621): highlight occurrences of a symbol in
  the current file.
- `textDocument/completion` (scalameta#640): override def completions from
  without the need to type "override def ".

It is my pleasure to welcome them to the team and I look forward to
working together with them to improve the Scala editing experience :)

olafurpg added a commit to olafurpg/metals that referenced this pull request Apr 10, 2019

Welcome Marek and Tomasz to the team!
As part of a new collaboration between the Scala Center and VirtusLab,
Marek and Tomasz will be contributing to Metals development for the
coming months. They have already contributed several impressive pull
requests:

- `textDocument/foldingRange` (scalameta#632): code folding that understands Scala syntax.
- `textDocument/documentHighlight` (scalameta#621): highlight occurrences of a symbol in
  the current file.
- `textDocument/completion` (scalameta#640): override def completions from
  without the need to type "override def ".

It is my pleasure to welcome them to the team and I look forward to
working together with them to improve the Scala editing experience :)

@olafurpg olafurpg added this to the Metals v0.5 - Mercury milestone Apr 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.