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.

Sign up for GitHub

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

Jump to bottom

Property completion #1957

Merged
merged 10 commits into from
Sep 28, 2020
Merged

Property completion #1957

merged 10 commits into from
Sep 28, 2020

Conversation

333fred
Copy link
Contributor

@333fred 333fred commented Sep 26, 2020

Based on #1941. In the process of playing around with the code, I discovered that I totally forgot about overriding properties when I implemented completion... 🤦‍♂️. This implements a more robust algorithm for finding the correct location in the changed text, and also handles property overrides correctly.

nohwnd and others added 9 commits September 14, 2020 15:16
@nohwnd
Fix completion on part of existing string
a329ce5
@333fred
Use text edit for main insert text
e21a232 
Because I misread the LSP spec when imlementing completion, I didn't realize that InsertTextFormat applied to both InsertText, and to the NewText property of the main TextEdit of a completion item. This latter version is much more robust to casing issues than using InsertText directly, and ensures that matching of completion items in the completion list works better as well. Fixes dotnet/vscode-csharp#4063.
@333fred
Add more tests for override spans within the original typed text, com…
255a883 
…ment with examples on vscode behavior for replacing ranges.
@nohwnd
Merge pull request #1 from 333fred/textedit-completion
252f79a 
Use text edit for main insert text
@nohwnd
Merge branch 'master' into completion-on-part-of-typed-text
6c2a86f
@333fred
Merge remote-tracking branch 'upstream/master' into cake-unit-tests
a44adb4 
* upstream/master:
  Make the regex tests windows only.
  Correct regex completion
@333fred
Update after merge
3805152 
* Remove incorrect assert
* Update the tests for the TextEdit changes
* Mark InsertText as obsolete to prevent new uses.
@333fred
Merge remote-tracking branch 'upstream/master' into parallel-completion
889441b 
* upstream/master:
  Docs: Remove redundant statement about logging
  Docs: Remove duplicated file, link to it instead
  Docs: File moved from wiki to doc folder
  Docs: Specify relationship to .NET Core SDK
  moved to .net 5.0 SDK 5.0.100-rc.1.20452.10
  added WarningsNotAsErrors for obsoletion to OmniSharp.Roslyn.CSharp.Tests/OmniSharp.Roslyn.CSharp.Tests.csproj
  mark autocomplete endpoint as obsolete
  restored regex test as conditional
@333fred
Support property override completion
568c9cc
@333fred
Copy link
Contributor Author

333fred commented Sep 26, 2020

The new part of this change is in commit 9. Commits 1-7 are based on #1941, and commit 8 is merging in master.

@333fred
Copy link
Contributor Author

333fred commented Sep 26, 2020

Alright, I really hope that nothing I did broke azdo package upload, because if it is something I did then everything is a lie.

filipw
filipw approved these changes Sep 27, 2020
View reviewed changes
Copy link
Member

@filipw filipw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks a lot! I feel a bit embarrassed that I didn't notice the property overrides thing..

@333fred
Copy link
Contributor Author

333fred commented Sep 27, 2020

thanks a lot! I feel a bit embarrassed that I didn't notice the property overrides thing..

Same...

@filipw
Copy link
Member

filipw commented Sep 28, 2020

@333fred looks like this fails on linux

    OmniSharp.Cake.Tests.CompletionFacts.ShouldGetAdditionalTextEditsFromOverrideCompletion [FAIL]
      Assert.Equal() Failure
      Expected: String[] ["public override bool", "public override int", "public override void", "public override void", "public override string"]
      Actual:   SelectIListIterator<CompletionItem, String> ["Cake.Core.IO.FilePath assemblyInfoPath)\n{\n    re"..., "public override int", "public override void", "public override void", "public override string"]
      Stack Trace:
          at OmniSharp.Cake.Tests.CompletionFacts.ShouldGetAdditionalTextEditsFromOverrideCompletion () [0x002a5] in <895d19e3d19548c59ba9489d2daa2025>:0 
      Output:

not sure if this is transient or not, let's see if the retry helps. Looks like Cake has some special host object property that is overrideable? @mholo65 not sure why it would affect Linux only though

@filipw
Copy link
Member

filipw commented Sep 28, 2020

hmm it passed now, but looking at the error above it looks like there is some race condition or other indeterministic state in the Cake tests. we can merge for now but we should keep an eye on this

@filipw filipw merged commit 2b5d02f into OmniSharp:master Sep 28, 2020
@bjorkstromm
Copy link
Member

@filipw thanks! I will dig into the Cake error and see if I find something obvious.

@333fred 333fred deleted the property-completion branch September 28, 2020 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@filipw filipw filipw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@333fred @filipw @bjorkstromm @nohwnd