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

Fix issue with missing completions after comment and newline. #814

Merged
merged 3 commits into from Jul 4, 2019

Conversation

@tgodzik
Copy link
Collaborator

commented Jul 1, 2019

Previously, completions after a comment and newline would yield no results.

Now we remove the comment from the name that PC gives us and this causes us to get all completions.

This is connected to a presentation compiler bug that would include comment as a legal identifier. We will try to fix that inside the compiler - the implementation here might not be most efficient, but it only happens in case of comments.

Fixes #813

@tgodzik tgodzik requested review from gabro and marek1840 Jul 1, 2019
@@ -369,9 +369,22 @@ class CompletionProvider(
val isTypeMember = kind == CompletionListKind.Type
params.checkCanceled()
val matchingResults = completions.matchingResults { entered => name =>
if (isTypeMember) CompletionFuzzy.matchesSubCharacters(entered, name)
else CompletionFuzzy.matches(entered, name)
/** @tgodzik presentation compiler bug

This comment has been minimized.

Copy link
@gabro

gabro Jul 1, 2019

Member

2 things:

  • can you link to the bug?
  • can you use the NOTE(tgodizk): notation for consistency with the rest of the codebase?

This comment has been minimized.

Copy link
@tgodzik

tgodzik Jul 2, 2019

Author Collaborator

No one reported the bug yet since ScalaIDE uses a different method for matching names and I don't think there is anybody else that uses PC as much. I plan to fix it though, so don't want to report it, just create a PR.

I will add the NOTE

This comment has been minimized.

Copy link
@tgodzik

tgodzik Jul 2, 2019

Author Collaborator

This bug: scala/scala#8193 uff...

This comment has been minimized.

Copy link
@tgodzik

tgodzik Jul 2, 2019

Author Collaborator

Let us see if I manage to get this merged - then we can just fix when upgrading to 2.12.9 and 2.13.1

@tgodzik tgodzik force-pushed the tgodzik:fix-issue-with-comments branch from a9a82f0 to a339e9c Jul 2, 2019
* in case we have a comment inline presentation compiler will see it as the name
* CompletionIssueSuite.issue-813 for more details
*/
val enteredFixed = if (entered.startsWith("$div$div")) {

This comment has been minimized.

Copy link
@marek1840

marek1840 Jul 2, 2019

Collaborator

Could you add comments to make those "$div$div" more readable? like:

  • // start of a comment: "//"
  • // entire one-line comment
…sults.

Now we remove the comment from the name that PC gives us and this causes us to get all completions.
@tgodzik tgodzik force-pushed the tgodzik:fix-issue-with-comments branch from a339e9c to ed902cf Jul 3, 2019
@tgodzik tgodzik force-pushed the tgodzik:fix-issue-with-comments branch from bf29084 to ec4ce61 Jul 3, 2019
@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 3, 2019

I also increased memory on the Appveyor windows tests since there should be at least 6 gb available on those machines. Let's see if it helps, some further issues might need us to profile them more thoroughly.

@tgodzik tgodzik requested review from marek1840 and gabro Jul 3, 2019
@gabro
gabro approved these changes Jul 3, 2019
@gabro

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Nice!

|
|object Main {
| Array(1, 1,10)
| .map(_ + 1) /* comment breaks completions */

This comment has been minimized.

Copy link
@gabro

gabro Jul 3, 2019

Member

Are completions supposed to be working when the comment is truly multiline?

This comment has been minimized.

Copy link
@tgodzik

tgodzik Jul 3, 2019

Author Collaborator

I think they should. The code with long comments inside compiles, so we should provide completions. Though, it would be weird to write a code like that for sure :O

This comment has been minimized.

Copy link
@ivanopagano

ivanopagano Jul 4, 2019

I guess you're allowed to break the cascade of code calls with multiline comments and continue with a .nextMethod(...) on a new line, after all the comments should be stripped by the compiler sooner or later...
It's a weird practice tho, breaking the flow of calls like that, I wouldn't encourage it...

@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 4, 2019

There seem to be more issues with OOO errors - I will try to investigate on a separate PR when I have time. And also might be good to test that on windows.

@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 4, 2019

Appveyor issues unrelated - we will try to work on those in the near future

@tgodzik tgodzik merged commit a61ee94 into scalameta:master Jul 4, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
scalameta.metals Build #20190703.4 succeeded
Details
@tgodzik tgodzik deleted the tgodzik:fix-issue-with-comments branch Jul 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.