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

Basic functionalities for LSP Server #31937

Open
wants to merge 11 commits into
base: master
from

Conversation

@ankitpriyarup
Copy link
Contributor

commented Sep 3, 2019

This PR provides functionality for: Function Signature, Markdown documentation & Document Link. I will create another PR later on covering rename functionality very soon.

@Geequlim
Copy link
Member

left a comment

The signatureHelper looks ok.
The documentLink should not resolve more than one files.


For documentations of builtin symbols

I'm not sure this is a good way to implement the builtin documentation support.

You dumping the documentaions of builtin symbols to markdown files from the DocData on the server side and display them in the client side. There would be a lot of markdown files in the output folders so we have to manage the versions and make sure they are not deleted by users. That is not so reliable IMO.

I suggest you send documentations in realtime from the server side every time the client request the documentation display (maybe format them to markdown before send). The the client decide how to display the recived data (may caching and open them as markdown file like you did or something else).


You'd better seperate the signatureHelp and documentLink from the markdown documentation code to different PRs.

@@ -75,9 +75,12 @@ class GDScriptWorkspace : public Reference {

String get_file_path(const String &p_uri) const;
String get_file_uri(const String &p_path) const;
String simplify_file_uri(const String &p_path) const;

This comment has been minimized.

Copy link
@Geequlim

Geequlim Sep 5, 2019

Member

This method seems only called in get_file_uri so you can put the implementation inside get_file_uri

String value = lines[i].substr(start_pos + 1, end_pos - start_pos - 1);

if (!value.begins_with("res://")) {
value = "res://" + value;

This comment has been minimized.

Copy link
@Geequlim

Geequlim Sep 5, 2019

Member

I don't think this is the right way to handle relative file paths.

How about this situation:
res://enermys/wolf.gd

extends 'enermy.gd'

'enermy.gd' should link to res://enermys/enermy.gd instead of res://enermy.gd.


value = GDScriptLanguageProtocol::get_singleton()->get_workspace()->get_file_uri(value);
// File is not outside project scope && exists
if (value.find(root_uri) != -1 && FileAccess::exists(value.replace(root_uri + "/", "res://"))) {

This comment has been minimized.

Copy link
@Geequlim

Geequlim Sep 5, 2019

Member

Why not call workspace->get_file_path here ?


void publish_diagnostics(const String &p_path);
void completion(const lsp::CompletionParams &p_params, List<ScriptCodeCompletionOption> *r_options);
Error get_signature_help(const lsp::TextDocumentPositionParams &p_params, String *r_signature_name, String *r_signature_doc, List<String> *r_signature_parameter, int *r_cur_active_parameter);
Error get_document_link(const lsp::DocumentLinkParams &p_params, List<lsp::DocumentLink> *r_links_parameter);

This comment has been minimized.

Copy link
@Geequlim

Geequlim Sep 5, 2019

Member

I don't think a path can link to more than one file so why need this function returns multi-values?

@@ -1467,7 +1621,7 @@ struct ServerCapabilities {
dict["signatureHelpProvider"] = signatureHelpProvider.to_json();
dict["codeLensProvider"] = false; // codeLensProvider.to_json();
dict["documentOnTypeFormattingProvider"] = documentOnTypeFormattingProvider.to_json();
dict["renameProvider"] = renameProvider.to_json();
// dict["renameProvider"] = renameProvider.to_json();

This comment has been minimized.

Copy link
@Geequlim

Geequlim Sep 5, 2019

Member

Do't commit unused code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.