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

Add Type Definition Provider API #19699

Merged
merged 5 commits into from
Feb 2, 2017
Merged

Add Type Definition Provider API #19699

merged 5 commits into from
Feb 2, 2017

Conversation

mjbvz
Copy link
Collaborator

@mjbvz mjbvz commented Feb 1, 2017

Fixes #10489

Adds a new vscode API to support type definition providers and adds an type definition provider for TypeScript

@mjbvz mjbvz requested a review from jrieken February 1, 2017 23:19
@mention-bot
Copy link

@mjbvz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jrieken and @alexandrudima to be potential reviewers.

@mjbvz mjbvz mentioned this pull request Feb 1, 2017
Closed
jrieken
jrieken requested changes Feb 2, 2017
View reviewed changes
src/vs/workbench/api/node/extHostLanguageFeatures.ts Outdated
constructor(
private documents: ExtHostDocuments,
private provider: vscode.DefinitionProvider)
{ }
Copy link
Member

Choose a reason for hiding this comment

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

restore previous coding style

src/vs/workbench/api/node/extHostLanguageFeatures.ts Outdated
this._documents = documents;
this._provider = provider;
}
constructor(
Copy link
Member

Choose a reason for hiding this comment

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

same

src/vs/workbench/api/node/extHostLanguageFeatures.ts Outdated
@@ -145,6 +137,26 @@ class ImplementationAdapter {
}
}

class TypeDefinitionAdapter {
constructor(
Copy link
Member

Choose a reason for hiding this comment

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

same

src/vs/vscode.d.ts
@@ -1668,6 +1668,24 @@ declare module 'vscode' {
}

/**
* The type definition provider defines the contract between extensions and
* the go to type definition feature.
Copy link
Member

Choose a reason for hiding this comment

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

Like the other language features this needs a section the doc (editing evolved) such that people has a clear understanding of what this is

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Opened #19731 to track this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's the PR: microsoft/vscode-docs#823

src/vs/editor/contrib/goToDeclaration/browser/goToDeclaration.ts Outdated
},
menuOpts: {
group: 'navigation',
order: 1.3
Copy link
Member

Choose a reason for hiding this comment

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

The order collides with the one above with means lex sort is applied which means order changes from language to language

src/vs/editor/contrib/goToDeclaration/browser/goToDeclaration.ts Outdated
menuOpts: {
group: 'navigation',
order: 1.3
}
Copy link
Member

Choose a reason for hiding this comment

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

In general we need to revisit the context menu because now it becomes more and more loaded. We will now have

  • Find References
  • Go to Definition
  • Peek Definition
  • Go to Implementation
  • Peek Implementation
  • Go to Type Definition
  • Peek Type Definition

which I think is a little messy already

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed.I think we should remove peek implementation and peek type definition from the menu.

mjbvz added 4 commits February 2, 2017 11:29
@mjbvz
Add Type Definition Provider API
3e0ee92 
Adds a new API to support type definition providers and adds an initial type definition provider for TypeScript
@mjbvz
Fix comment
d39aa8f
@mjbvz
Addressing comments
784d77d
@mjbvz
Fixer menu option ordering
16484f8
@mjbvz mjbvz force-pushed the add-type-definition-provider branch from 7959132 to 16484f8 Compare February 2, 2017 19:38
mjbvz added a commit to mjbvz/vscode-docs that referenced this pull request Feb 2, 2017
@mjbvz
Add Go to Implementation and Go to Type Definition Sections
cd10ba6 
From microsoft/vscode#19699
@mjbvz mjbvz mentioned this pull request Feb 2, 2017
Merged
@mjbvz mjbvz added this to the February 2017 milestone Feb 2, 2017
@mjbvz mjbvz merged commit a87586c into microsoft:master Feb 2, 2017
@jrieken
Copy link
Member

jrieken commented Feb 3, 2017

@mjbvz We should maybe revisit this or think more about this. I wonder if it would make more sense to not have a separate provider here but to make this an option the existing DefinitionProvider.

@mjbvz
Copy link
Collaborator Author

mjbvz commented Feb 4, 2017

@jrieken I'd be fine with that. The only arguments I see in having a separate TypeDefinitionProvider interface:

  • It is more consistent with the existing DefinitionProvider and ImplementationProvider api. Each of these power a single piece of editor functionality, such as go to definition or go to implementation
  • Adding an optional provideTypeDefinition method on DefinitionProvider means that an extension could not register only as a type definition provider. They would have to implement provideDefinition as well, even if they return empty results

Let me know if you think we refactor the api this way or if you have any alternate design proposals.

@Xanewok Xanewok mentioned this pull request Jul 14, 2018
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jrieken jrieken jrieken requested changes

Assignees

@mjbvz mjbvz

Labels
None yet
Projects
None yet
Milestone
February 2017
Development

Successfully merging this pull request may close these issues.

Go to inferred type definition via keyboard shortcut
4 participants
@mjbvz @mention-bot @jrieken @msftclas