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

Preselect feature in the completion item #2388

Merged
merged 11 commits into from
Aug 28, 2018
Merged

Preselect feature in the completion item #2388

merged 11 commits into from
Aug 28, 2018

Conversation

akshita31
Copy link
Contributor

No description provided.

@akshita31 akshita31 requested review from DustinCampbell, rchande and colombod and removed request for DustinCampbell and rchande June 21, 2018 22:15
rchande
rchande suggested changes Jun 21, 2018
View reviewed changes
Copy link

@rchande rchande left a comment

Choose a reason for hiding this comment

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

I "approve" of this PR (module my comment), but am marking "request changes" so we don't merge this before we should.

src/features/completionItemProvider.ts Outdated
@@ -66,6 +66,8 @@ export default class OmniSharpCompletionItemProvider extends AbstractSupport imp
completion.commitCharacters = response.IsSuggestionMode
? OmniSharpCompletionItemProvider.CommitCharactersWithoutSpace
: OmniSharpCompletionItemProvider.AllCommitCharacters;

completion.preselect = response.Preselect;
Copy link

Choose a reason for hiding this comment

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

It'd be cool if we could test this somehow...

package.json Outdated
@@ -269,7 +269,7 @@
}
],
"engines": {
"vscode": "^1.18.0"
"vscode": "*"
Copy link

Choose a reason for hiding this comment

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

As discussed let's not merge this until the feature moves into the latest stable build.

@codecov
Copy link

codecov bot commented Jun 25, 2018 •
edited
Loading

Codecov Report

Merging #2388 into master will increase coverage by 0.24%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2388      +/-   ##
==========================================
+ Coverage   64.34%   64.58%   +0.24%     
==========================================
  Files          90       90              
  Lines        4120     4123       +3     
  Branches      592      593       +1     
==========================================
+ Hits         2651     2663      +12     
+ Misses       1294     1288       -6     
+ Partials      175      172       -3
Flag Coverage Δ
#integration 53.84% <92.85%> (+0.59%) ⬆️
#unit 84.52% <ø> (ø) ⬆️
Impacted Files Coverage Δ
src/omnisharp/protocol.ts 85.49% <ø> (ø) ⬆️
src/features/completionItemProvider.ts 87.87% <92.85%> (+10.1%) ⬆️
src/features/diagnosticsProvider.ts 73.18% <0%> (-3.63%) ⬇️
src/omnisharp/requestQueue.ts 81.81% <0%> (-3.04%) ⬇️
src/omnisharp/server.ts 72.43% <0%> (+0.7%) ⬆️
src/features/codeLensProvider.ts 48.54% <0%> (+1.94%) ⬆️
src/features/documentSymbolProvider.ts 100% <0%> (+4.87%) ⬆️
src/features/documentation.ts 47.61% <0%> (+9.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 250f028...c0718c8. Read the comment docs.

@TheRealPiotrP
Copy link

This PR drops 30% of our code coverage. What's going on?

@akshita31
Copy link
Contributor Author

@TheRealPiotrP The integration tests are failing because the desired vscode version has not been released. Once that is in place, we can get the real coverage analysis.

@rchande
Copy link

rchande commented Jun 27, 2018

@akshita31 @TheRealPiotrP in #2360 I changed the version of vs code used to run the integration tests. You might need to do something similar to get tests passing.

@akshita31
Copy link
Contributor Author

Blocked on : microsoft/vscode#53749

@SirIntruder
Copy link

Did latest vscode version fix the issue?

DustinCampbell
DustinCampbell reviewed Aug 28, 2018
View reviewed changes
Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

Seems good, but see my comment about the engine change.

package.json Outdated
@@ -269,7 +269,7 @@
}
],
"engines": {
"vscode": "^1.23.0"
"vscode": "^1.26.1"
Copy link
Member

Choose a reason for hiding this comment

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

Note that this will mean that users will need to install at least 1.26.1 in order to install C# for VS Code. Is that intentional? Does this API only appear in 1.26.1? Or, is it in an earlier version (e.g. 1.26.0).

@akshita31 akshita31 merged commit 9b2e06e into dotnet:master Aug 28, 2018
@akshita31 akshita31 deleted the preselect branch August 28, 2018 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DustinCampbell DustinCampbell DustinCampbell left review comments

@rchande rchande rchande approved these changes

@colombod colombod Awaiting requested review from colombod

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

Successfully merging this pull request may close these issues.
5 participants
@akshita31 @TheRealPiotrP @rchande @SirIntruder @DustinCampbell