-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
CallHierarchyService Plugin API #3765 #6924
CallHierarchyService Plugin API #3765 #6924
Conversation
013a6ac
to
3f35234
Compare
wanted to test with https://github.com/rmi22186/vscodeextension/blob/926c5c045e3274a0572f4a07ca0adf6d2fdbd949/call-hierarchy-sample/README.md but failed to do (I think there is an error unrelated to this PR) |
3eee615
to
615f7c8
Compare
What is the error? |
error was |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
16371d6
to
95dde3e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've only reviewed that MS code does not spread over our codebase. Please keep in the plugin system as possible, if you need in callhierarchy move it there, but better use our implementations.
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 | ||
********************************************************************************/ | ||
|
||
export * from './language-selector'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move ilanguage-selectro code to @theia/callhierarchy
or to @theia/langauges
from @theia/core
, plugin system still can access it from there
core is only about the application framework and shell and anything which require to support it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about where to put it and decided against "callhierarchy". It is really not directly related to call hierarchy, even in the plugin-ext usage it's not related to call hierarchy. We rely on monaco to do language-selector related stuff for other services like code completion. It's utility code related to language implementation. Maybe @theia/languages/common would fit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, let's do @theia/languages/common
@@ -21,7 +21,7 @@ | |||
*--------------------------------------------------------------------------------------------*/ | |||
'use strict'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have Path
in core, please use it instead, we don't need another implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both path.ts
and strings.ts
are things that are used in code copied from VS Code as requirements for glob.ts
. That is also why I copied that stuff to common/language-selector
and did not put it in common
. Using the Path
object from core would have meant to rewrite a lot of code that his not related to this PR (I just moved it). Some of the code seems to do the same thing, but I feared there might be subtle differences that might break the glob code from VS Code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, let's move it out of core to languages as well
@@ -14,7 +14,7 @@ | |||
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 | |||
********************************************************************************/ | |||
|
|||
// copied from https://github.com/Microsoft/vscode/blob/bf7ac9201e7a7d01741d4e6e64b5dc9f3197d97b/src/vs/base/common/strings.ts | |||
// based on https://github.com/Microsoft/vscode/blob/bf7ac9201e7a7d01741d4e6e64b5dc9f3197d97b/src/vs/base/common/strings.ts | |||
/*--------------------------------------------------------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is already strings.ts file in core, could it be merged? I see a lot of utility code copied, we don't need it in core and it does not seem that all these code is used, could we remove all unused functions and types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've only reviewed that MS code does not spread over our codebase.
wouldn't merging the two files that go against that goal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we move out it form core, it would be fine with me
@@ -2009,3 +2009,36 @@ export enum WebviewPanelTargetArea { | |||
Right = 'right', | |||
Bottom = 'bottom' | |||
} | |||
export class CallHierarchyItem { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope this types are just copied without any modifications from VS Code codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I shortened the code considerably using parameter properties. Since we already have documentation for the properties in theia.d.ts (where it's copied verbatim), no reason not to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope to be able to put this file next to counterpart to VS Code and see differences like explained in this PR: https://github.com/eclipse-theia/theia/pull/6903/files#diff-905508836c18f47e2fc9e75198b465e8R21-R32
I would appreciate if we just keep types and don't change anything to simplify synching.
there should not be commits fixing issues introduced by a PR: https://github.com/eclipse-theia/theia/blob/master/doc/pull-requests.md#checklist-commit-history |
I usually keep the "fixup commits" until the comments are all addressed and then fold them back into the original commit. Is that acceptable? |
@akosyakov I have replied to a bunch of your comments and would need a response. |
f9897f9
to
fe1ab01
Compare
@akosyakov I addressed your comments. |
I tried with callhierarchy example (by fixing the example by removing stuff which is not yet implemented in theia about filesystem to initialize the txt file as it doesn't matter for this test as I can just copy/paste manually the text content) |
@benoitf can you provide your fixed .visix? Also, can you provide some log output? The behaviour would be the same if the extension just never got activated. |
@tsmaeder sure, custom vsix is there: I've removed showSampleText method and provide the in the log I can see
but nothing reported when clicking on |
@benoitf do you have any error messages in the browser logs? |
yes:
|
Thx @benoitf |
@benoitf problem lies with document-data.ts#validatePosition(): if (!(position instanceof Position)) {
throw new Error('Invalid argument');
} where |
@tsmaeder AFAIK we can only use interfaces from theia.d.ts and as Position is a class we can only use Position from types-impl. |
cd0a369
to
3b2d08c
Compare
Issue eclipse-theia#3765 Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com> Signed-off-by: Thomas Mäder <tmader@redhat.com>
3b2d08c
to
a982e56
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have not tried, but code and dependency wise it looks good to me.
Signed-off-by: Thomas Mäder tmader@redhat.com
What it does
Introduces call hierarchy API into theia.d.ts (see https://code.visualstudio.com/api/references/vscode-api#languages). Support for
SymbolTag
has been omitted, since we do not support it for other features.Only necessary changes have been done the UI. In particular, no support for "outgoing calls" has been added. In consequence (for lack of easy testing) the codepath producing "outgoing calls" has not been added.
Since the VS Code API is based on
LanguageSelector
, it was easier to break the API forCallHierarchyService
. I believe there aren't any implementors outside of the typescript extension.How to test
Build one of the standard examples, removing Java support from the package.json. Then add the latest vscode-java extension (https://github.com/redhat-developer/vscode-java/releases) to /plugins. Open a typescript project to test the extension API, open a Java project to test the VS Code API.
Review checklist
Reminder for reviewers