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

Definition link API #52230

Merged
merged 4 commits into from
Jun 20, 2018
Merged

Definition link API #52230

merged 4 commits into from
Jun 20, 2018

Conversation

mjbvz
Copy link
Contributor

@mjbvz mjbvz commented Jun 18, 2018

Add a new DefinitionLink type. This type allows definition providers to return additional metadata about a definition, such as the defining span.

Hook up this new provider for typescript

This PR replaces #48001

Fixes #10037

@mjbvz mjbvz self-assigned this Jun 18, 2018
@mjbvz mjbvz requested a review from jrieken June 18, 2018 20:26
@mjbvz mjbvz mentioned this pull request Jun 18, 2018
origin?: Range;
uri: Uri;
range: Range;
selectionRange?: Range;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

using these field names make DefinitionLink compatible with Location

Copy link
Member

Choose a reason for hiding this comment

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

sneaky... we could also make origin mandatory and use the position of the request as origin

Copy link
Member

Choose a reason for hiding this comment

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

or did you plan that as non-breaking API evolution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Main reason for making it optional is that I think it makes sense for implementers:

  • When origin is provided: always use this value.
  • When origin is undefined: use the word range at the request position as the origin

If origin is not optional, I believe many extensions would have to compute the word range themselves

@jrieken
Copy link
Member

jrieken commented Jun 19, 2018

@mjbvz Please fix merge conflicts

Copy link
Member

@jrieken jrieken left a comment

Choose a reason for hiding this comment

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

nits - feel free to merge

}
return undefined;
});
}

private static convertDefinitionLink(position: IPosition, value: vscode.Location | vscode.DefinitionLink): modes.DefinitionLink {
Copy link
Member

Choose a reason for hiding this comment

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

position is unused. move to extHostTypeConverter?

origin?: Range;
uri: Uri;
range: Range;
selectionRange?: Range;
Copy link
Member

@jrieken jrieken Jun 20, 2018

Choose a reason for hiding this comment

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

One a second thought I wonder if defaulting DefinitionLink.range to Location.range is the right thing... It feels more like the selectionRange cos that's how it is currently used. It's also OK to leave this out of this PR but something we should likely do when we optimise how the preview hover is computed.

let wordRange: Range;
if (result.origin) {
const range = result.origin;
wordRange = new Range(range.startLineNumber, range.startColumn, range.endLineNumber, range.endColumn);
Copy link
Member

Choose a reason for hiding this comment

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

fyi - Range.lift does the job

Add a new `DefinitionLink` type. This type allows definition providers to return additional metadata about a definition, such as the defining span.

Hook up this new provider for typescript

This PR replaces microsoft#48001
- Use lift
- Remove unused param
@mjbvz
Copy link
Contributor Author

mjbvz commented Jun 20, 2018

@jrieken Thanks. I've made the suggested changes and will merge this into proposed

I may be confusing range and selectionRange though. For a class definition, should range be the full range of a class body, while selection range would be the range of the class name? Or is it the reverse?

@mjbvz mjbvz merged commit 0532c31 into microsoft:master Jun 20, 2018
@jrieken
Copy link
Member

jrieken commented Jun 21, 2018

Yeah, range should be the full range but I think todays implementations return something that closer to the selectionRange

@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend language API to allow sourceRange in Go to Definition
2 participants