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

Add completion to add all abstract members #1031

Merged
merged 6 commits into from Nov 11, 2019

Conversation

@ckipp01
Copy link
Member

ckipp01 commented Oct 30, 2019

2019-10-30 21 44 17

So I believe I have the implemented correctly now that it will work for all abstract members -- defs, vals, and vars. If you have 5, but 2 implemented, then the implement all will implement the remaining 3.

I am not sure about a few things though...

  1. The way I did the actual text edit with the strings of the other text edits
  2. I wasn't positive about what you mentioned in the issue about this #1008 (comment), but I made a test with that scenario, and it seems to work as expected?
  3. Other tests that I may be missing.

This should close #1029 and also address #1008. However, regarding #1008, I'm not sure if we also want to implement it as well where if you are instantiating a class or something that has the abstract members, it will gives you the option to automatically create the class not just with an empty {} but rather with all the members. I wasn't sure if that should be grouped with this PR or not.

@ckipp01 ckipp01 force-pushed the ckipp01:feature/all-abstract-members branch from 188e02a to 895cb0e Oct 30, 2019
@ckipp01

This comment has been minimized.

Copy link
Member Author

ckipp01 commented Oct 30, 2019

Seeing the full test suite run I realized I'm missing some cases like the override keywords when the members are in abstract classes etc. I'll address those. Sorry, I should have looked closer at those before issuing the pr. For now, I'll just say I knew they were failing and just wanted some earlier feedback 😏 .

Copy link
Member

olafurpg left a comment

Great progress! Looking good, mostly a few minor comments related to style and performance

@ckipp01 ckipp01 force-pushed the ckipp01:feature/all-abstract-members branch 2 times, most recently from 6082cb9 to e6d6f64 Nov 2, 2019
@ckipp01 ckipp01 requested a review from olafurpg Nov 3, 2019
@ckipp01

This comment has been minimized.

Copy link
Member Author

ckipp01 commented Nov 3, 2019

I think it may be best to have someone take a look at the what is going on with the tests and the indentation. I'm sort of just going in circles. I can't seem to figure out why the behavior is different locally than in the test, but I can't get them to act the same.

Also, can I get the the ci re-triggered? I'm getting an odd failure that I can't seem to mimic locally, and I'm curious if it was a flook.

@gabro

This comment has been minimized.

Copy link
Member

gabro commented Nov 3, 2019

@ckipp01 I've taken a quick look, and it seems the indentation in the newText field of the completion item is not correct. For example, here's the newText in the simple-edit test case:

"def foo: Int = ${0:???}\ndef bar: Int = ${0:???}"

Notice the absence of spaces after \n.

So the rendering in tests seems correct. The fact that you see a correct behavior when testing in the editor could be due to the editor itself

@ckipp01

This comment has been minimized.

Copy link
Member Author

ckipp01 commented Nov 3, 2019

Notice the absence of spaces after \n

hmm, yea I do see that. Any idea why then when I do grab the indentation and add it after the \n that it double indents in the editors?

@gabro

This comment has been minimized.

Copy link
Member

gabro commented Nov 3, 2019

@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Nov 4, 2019

@ckipp01 this seems relevant microsoft/language-server-protocol#83

We could drop the indentation when it's being used in VS Code. The setting can be specified in MetalsServerConfig.

I think I've seen the comment about somewhere, so I might be repeating myself, but I think this will be fine do for VS Code.

@gabro

This comment has been minimized.

Copy link
Member

gabro commented Nov 4, 2019

@tgodzik yep, I agree that’s a good solution. Apparently coc.vim does the same, so it’s not only vscode.
We briefly discussed this on Discord yesterday

@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Nov 4, 2019

The failing test seems to be breaking only for 2.13.0, so best to change the Scala version in sbt and run the test. It might be fixed in 2.13.1, in that case I would just ignore it for 2.13.0. We do it in a couple of different cases.

sbt
> ++2.13.0
> cross/testOnly -- tests.pc.CompletionOverrideConfigSuite

@gabro discord! That's where I have seen it! 😆

@ckipp01

This comment has been minimized.

Copy link
Member Author

ckipp01 commented Nov 4, 2019

So tonight I went on a little journey to see what editors input this "magic indent" and we know that VS Code did as well as coc.nvim.

Atom also seems to add it...
2019-11-04 20 40 35

Emacs seems to not add it...
2019-11-04 21 15 11

but then I came across something really bizarre... I haven't really used Sublime before and I gave it a shot, and I could not figure out why I couldn't get the "Implement all members" to appear. Randomly, I somehow just typed e which will consistently bring up the "Implement all members"... but then when it is selected, it just inputs that text. Any idea what in the world is going on, and why that is happening in Sublime?
2019-11-04 21 32 49

@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Nov 5, 2019

@ckipp01 Thanks for looking into this! Seems Sublime uses label as text to be inserted, it seems to be a bug inside the plugin. We might want to use insertText (or label if all else fails) for Sublime itself. But not sure if it's a large issue right now, since that is a client issue.

For any formatting issues we might want to add formatting command to be run after the completion, which should solve most issues.

/**
	 * An optional command that is executed *after* inserting this completion. *Note* that
	 * additional modifications to the current document should be described with the
	 * additionalTextEdits-property.
	 */
	command?: Command;
@gabro

This comment has been minimized.

Copy link
Member

gabro commented Nov 5, 2019

Paging also @ayoub-benali about the Sublime LSP bug

@ayoub-benali

This comment has been minimized.

Copy link
Contributor

ayoub-benali commented Nov 5, 2019

@ckipp01 have you tried using this setting in the LSP plugin ?
"prefer_label_over_filter_text": true

@ckipp01

This comment has been minimized.

Copy link
Member Author

ckipp01 commented Nov 5, 2019

@ckipp01 have you tried using this setting in the LSP plugin ?

Yea, I have that setting set to true.

@ayoub-benali

This comment has been minimized.

@ayoub-benali

This comment has been minimized.

Copy link
Contributor

ayoub-benali commented Nov 5, 2019

@ckipp01 there could be a regression in the plugin then. For example the Exhaustive match feature used to work and now it doesn't for me. I assume this PR behaves the same

@ckipp01

This comment has been minimized.

Copy link
Member Author

ckipp01 commented Nov 5, 2019

@ckipp01 there could be a regression in the plugin then. For example, the Exhaustive match feature used to work and now it doesn't for me. I assume this PR behaves the same

Ironically, when testing this, the exhaustive match worked correctly for me which just confused me more about why this one was behaving differently.

@ayoub-benali

This comment has been minimized.

Copy link
Contributor

ayoub-benali commented Nov 5, 2019

ok I will investigate the sublime text behavior later today

@ckipp01

This comment has been minimized.

Copy link
Member Author

ckipp01 commented Nov 6, 2019

So I took a stab at adding in the isMagicIndentClient which I use to determine whether the indent should be added or not. This now allows the tests to act normal and have the spacing you would expect and also to work in the editors that don't seem to do the magic indent. However, I screwed something up with the way the value is being set because even in the editors where I set it to true, it seems that the value is never actually being set and always defaulting. So, I know I'm missing something. Help me Obi Wan Kenobi, what am I missing?

@gabro

This comment has been minimized.

Copy link
Member

gabro commented Nov 7, 2019

even in the editors where I set it to true, it seems that the value is never actually being set and always defaulting

@ckipp01 how are you testing this? The configuration is based on the metals.client property sent by the clients

@ckipp01

This comment has been minimized.

Copy link
Member Author

ckipp01 commented Nov 7, 2019

@ckipp01 how are you testing this? The configuration is based on the metals.client property sent by the clients

I use publishLocal, and then restart the server after making sure VS Code is set to the latest SNAPSHOT. It's for sure using the latest snapshot (I tested with having it input random text to make sure it was using the latest). Same with vim, I bootstrap the latest snapshot before opening the project to test.

@gabro

This comment has been minimized.

Copy link
Member

gabro commented Nov 7, 2019

@ckipp01 I don't have brilliant ideas, but the config is initialized here

val config = MetalsServerConfig.default
val server = new MetalsLanguageServer(

so you may want to check what happens there and whether the expected system properties are set. You can use a scribe.info() as a glorified println 🤓

@ckipp01 ckipp01 force-pushed the ckipp01:feature/all-abstract-members branch from a45bd9f to 4a96189 Nov 7, 2019
@ckipp01

This comment has been minimized.

Copy link
Member Author

ckipp01 commented Nov 7, 2019

Alright, I believe I have it all figured out now. I was correctly setting the value based on the client... but not passing it into the compiler options like I needed to. That's now fixed and I address the naming things you brought up @olafurpg. Playing around with it locally with VS Code, emacs, atom, and vim all seem to be working as expected!

@ckipp01 ckipp01 requested a review from olafurpg Nov 8, 2019
@ckipp01 ckipp01 added the feature label Nov 8, 2019
@ckipp01 ckipp01 requested a review from gabro Nov 8, 2019
Copy link
Member

olafurpg left a comment

The test failure seems legitimate

2019-11-07T20:37:28.3751896Z ===========
2019-11-07T20:37:28.3752152Z => Obtained
2019-11-07T20:37:28.3752409Z ===========
2019-11-07T20:37:28.3752659Z     """|package example
2019-11-07T20:37:28.3752919Z        |
2019-11-07T20:37:28.3753267Z        |trait Foo {
2019-11-07T20:37:28.3753610Z        |  def foo: Int
2019-11-07T20:37:28.3753865Z        |  val bar: Int
2019-11-07T20:37:28.3754171Z        |  var car: Int
2019-11-07T20:37:28.3754456Z        |}
2019-11-07T20:37:28.3754828Z        |object Main {
2019-11-07T20:37:28.3755164Z        |  val x = new Foo {
2019-11-07T20:37:28.3755424Z        |    def foo: Int = ${0:???}
2019-11-07T20:37:28.3756297Z        |    val bar: Int = ${0:???}
2019-11-07T20:37:28.3757119Z        |    def car: Int = ${0:???}
2019-11-07T20:37:28.3757534Z        |  }
2019-11-07T20:37:28.3757820Z        |}
2019-11-07T20:37:28.3758114Z 
2019-11-07T20:37:28.3758340Z 
2019-11-07T20:37:28.3758670Z =======
2019-11-07T20:37:28.3759035Z => Diff
2019-11-07T20:37:28.3759569Z =======
2019-11-07T20:37:28.3760233Z ---∙
2019-11-07T20:37:28.3760789Z +++∙
2019-11-07T20:37:28.3761374Z @@ -11,3 +11,3 @@
2019-11-07T20:37:28.3761741Z      val bar: Int = ${0:???}
2019-11-07T20:37:28.3762210Z -    def car: Int = ${0:???}
2019-11-07T20:37:28.3762583Z +    var car: Int = ${0:???}
2019-11-07T20:37:28.3762896Z    }

I'm not sure what Scala version that's on, I think 2.11.12. Previously, the test names in cross/test included the Scala version but that seems to have regressed since we moved to GitHub Actions 🤔

@olafurpg olafurpg changed the title add completion to add all abstract members Add completion to add all abstract members Nov 8, 2019
@ckipp01

This comment has been minimized.

Copy link
Member Author

ckipp01 commented Nov 8, 2019

The test failure seems legitimate

2019-11-07T20:37:28.3751896Z ===========
2019-11-07T20:37:28.3752152Z => Obtained
2019-11-07T20:37:28.3752409Z ===========
2019-11-07T20:37:28.3752659Z     """|package example
2019-11-07T20:37:28.3752919Z        |
2019-11-07T20:37:28.3753267Z        |trait Foo {
2019-11-07T20:37:28.3753610Z        |  def foo: Int
2019-11-07T20:37:28.3753865Z        |  val bar: Int
2019-11-07T20:37:28.3754171Z        |  var car: Int
2019-11-07T20:37:28.3754456Z        |}
2019-11-07T20:37:28.3754828Z        |object Main {
2019-11-07T20:37:28.3755164Z        |  val x = new Foo {
2019-11-07T20:37:28.3755424Z        |    def foo: Int = ${0:???}
2019-11-07T20:37:28.3756297Z        |    val bar: Int = ${0:???}
2019-11-07T20:37:28.3757119Z        |    def car: Int = ${0:???}
2019-11-07T20:37:28.3757534Z        |  }
2019-11-07T20:37:28.3757820Z        |}
2019-11-07T20:37:28.3758114Z 
2019-11-07T20:37:28.3758340Z 
2019-11-07T20:37:28.3758670Z =======
2019-11-07T20:37:28.3759035Z => Diff
2019-11-07T20:37:28.3759569Z =======
2019-11-07T20:37:28.3760233Z ---∙
2019-11-07T20:37:28.3760789Z +++∙
2019-11-07T20:37:28.3761374Z @@ -11,3 +11,3 @@
2019-11-07T20:37:28.3761741Z      val bar: Int = ${0:???}
2019-11-07T20:37:28.3762210Z -    def car: Int = ${0:???}
2019-11-07T20:37:28.3762583Z +    var car: Int = ${0:???}
2019-11-07T20:37:28.3762896Z    }

I'm not sure what Scala version that's on, I think 2.11.12. Previously, the test names in cross/test included the Scala version but that seems to have regressed since we moved to GitHub Actions 🤔

I was assuming the test failure was what @tgodzik was referring to up above when he said

The failing test seems to be breaking only for 2.13.0, so best to change the Scala version in sbt and run the test. It might be fixed in 2.13.1, in that case I would just ignore it for 2.13.0. We do it in a couple of different cases.

as I can seem to reproduce it locally when running the tests or when actually testing the functionality.

@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Nov 8, 2019

I was assuming the test failure was what @tgodzik was referring to up above when he said

Did you manage to confirm that it only breaks for 2.13.0? I think the failure I saw was a bit different, so might be a different issue altogether.

Edit: This failure is only for 2.11.12, so something else is failing now.

@ckipp01

This comment has been minimized.

Copy link
Member Author

ckipp01 commented Nov 9, 2019

I was able to fix the issue where imports weren't being added if they were necessary for the implement all members.

I have verified that the tests in cross that are failing only fail for 2.11.12. The isVar seems to not be working the same way. We had a conversation regarding this here https://discordapp.com/channels/632642981228314653/632652693013528589/642698819967909899 on discord. I'm sort of at a loss of how to fix it.

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Nov 10, 2019

@ckipp01 do the tests maybe get fixed if you rebase on top of master? The issue with vars got fixed in #1056 by @gabro

@gabro

This comment has been minimized.

Copy link
Member

gabro commented Nov 10, 2019

yep, @ckipp01 confirmed in the other PR that tests pass when rebased 🎉

@ckipp01 ckipp01 force-pushed the ckipp01:feature/all-abstract-members branch from ad7138f to d86f1fa Nov 10, 2019
@ckipp01 ckipp01 requested a review from olafurpg Nov 10, 2019
@ckipp01

This comment has been minimized.

Copy link
Member Author

ckipp01 commented Nov 10, 2019

Soooo happy to see all the green check marks finally, ha! Thanks for the work on #1056 @gabro that seemed to do the trick!

Copy link
Member

olafurpg left a comment

LGTM 👍 Excited to try this out!

@olafurpg olafurpg merged commit f3d517c into scalameta:master Nov 11, 2019
9 checks passed
9 checks passed
Windows unit tests
Details
Linux unit tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Slow tests
Details
Scala cross tests
Details
Scalafmt/Scalacheck/Docs
Details
@ckipp01 ckipp01 deleted the ckipp01:feature/all-abstract-members branch Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.