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

Move project.json dependency completion & hover to vscode-omnisharp #1236

Merged

Conversation

DustinCampbell
Copy link
Member

This change adds the project.json completion and hover support to C# for Visual Studio Code, allowing it to be removed from VS Code.

This PR supersedes #224.

cc @aeschli

@DustinCampbell
Copy link
Member Author

Hey @aeschli: Both jsonc-parser and request-light appear to be designed for VS Code extension authors. Is there a list somewhere of packages intended for VS Code extensions?

@DustinCampbell DustinCampbell mentioned this pull request Feb 13, 2017
5 tasks
@@ -33,9 +33,11 @@
"fs-extra": "^1.0.0",
"http-proxy-agent": "^1.0.0",
"https-proxy-agent": "^1.0.0",
"jsonc-parser": "^0.3.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to update our ThirdPartyNotices.txt for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. That's actually produced by the VS Code team and published by @aeschli here.

@DustinCampbell
Copy link
Member Author

Any comments here @aeschli?

@aeschli
Copy link
Contributor

aeschli commented Feb 15, 2017

@DustinCampbell Good point, we don't have such a list. I created microsoft/vscode-docs#846

@aeschli
Copy link
Contributor

aeschli commented Feb 15, 2017

jsonc-parser and request-light are both owned by Microsoft. No need to list in the ThirdPartyNotices.

@DustinCampbell
Copy link
Member Author

@gregg-miskelly: Any other feedback? If not, I'm going to go ahead and take this so that @aeschli can remove the code on the VS Code side for their March release.

@aeschli: Currently, I find that this causes duplicate hover tooltips due to having HoverProviders in both VS Code and C# for VS Code. However, the experience isn't too terrible, and it's a point in time problem.

Copy link
Contributor

@gregg-miskelly gregg-miskelly left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM as far as I understand it.

'version': '{{1.0.0-*}}',
'dependencies': {},
'frameworks': {
'dnx451': {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update these to something more modern?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. We can do it later though. These will still work for now.

@DustinCampbell DustinCampbell merged commit c4c977c into dotnet:master Feb 16, 2017
@DustinCampbell
Copy link
Member Author

@aeschli: Could you cc me on the corresponding change in VS Code?

aeschli added a commit to microsoft/vscode that referenced this pull request Feb 16, 2017
@aeschli
Copy link
Contributor

aeschli commented Feb 16, 2017

@DustinCampbell I removed the project.json support from VSCode. This will be part of the February release (in about 2 weeks)

@DustinCampbell
Copy link
Member Author

Got it. Thanks!

@DustinCampbell
Copy link
Member Author

Also, @aeschli, sorry that took so long!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants