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

Added new language server options and functionality. #3161

Merged

Conversation

pristinesource
Copy link
Contributor

Added new extended diagnostic code information to the language server.
-- It must be enabled via a command line switch.
Added telemetry data for language server initialization and operation.
Added verbose log messages for language server.
-- It must be enabled via a command line switch.

I will comment on the code changes directly in the diff.

Added new extended diagnostic code information to the language server.
 -- It must be enabled via a command line switch.
Added telemetry data for language server initialization and operation.
Added verbose log messages for language server.
 -- It must be enabled via a command line switch.
* @var int
* @readonly
*/
public $shortcode;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this so it can be used to send the error code to the client.


* @return Promise<void>
*/
public function logMessage(string $message, int $type = 4, string $method = 'window/logMessage'): Promise
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method allows us to send notifications with the LogMessageParams type as the params for the message. The method is customizable, because we use this same method to send telemetry data using the telemetry/event method which can use anything for its params.

@@ -200,16 +202,25 @@ public function initialize(
return call(
/** @return \Generator<int, true, mixed, InitializeResult> */
function () use ($capabilities, $rootPath, $processId) {
$this->verboseLog("Initializing...");
$this->clientStatus('initializing');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These client statuses should be known strings, so the client can use them and provide UI updates as needed, such as status appropriate icons, or extended status messages.

$diagnostic->code = [
"value" => $code,
"target" => $issue_data->link,
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here we send the detailed diagnostic code with the help link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@muglug here is where it uses the short code to sent to the client. Right now it pads it to 3 chars and prepends 'PS' to it so a code of 22 would be 'PS022'. We can make it be anything you like, even making the code sent to the client be the link; we would just set the link to both "value" and "target". We need to send the extended code structure instead of just a string in order to make it clickable in the IDE.

echo 'Psalm ' . PSALM_VERSION . PHP_EOL;
exit;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this up to allow for an early exit since they just want the version. There is no need to check other stuff before this.

@pristinesource
Copy link
Contributor Author

pristinesource commented Apr 16, 2020

I have a PR for the vscode extension that uses these updates so you can see how they would be used.
psalm/psalm-vscode-plugin#12 psalm/psalm-vscode-plugin#13

@pristinesource
Copy link
Contributor Author

Oh, it looks like I forgot to update the tests ... I will get that fixed up.

@muglug
Copy link
Collaborator

muglug commented Apr 16, 2020

Thanks! Any reason you need the short code? Won’t the uri field provide all the data you need there?

@pristinesource
Copy link
Contributor Author

pristinesource commented Apr 16, 2020

@muglug, yes vscode (and other IDEs) need the error code to associate with the error. Most of the tim and IDE (vscode does this) requires an error code to attach the link to in the UI.

Here is an example of what it looks like in VSCode (I had to redact some areas of the UI because they are for my work):

Screenshot 2020-04-16 13 04 10

The part in the parenthesis is the code, and is also the link.

@pristinesource
Copy link
Contributor Author

The failing test with AppVeyor is not an issue with my code, it looks like it is having trouble uploading a file. 🤔

@pristinesource
Copy link
Contributor Author

@muglug, also if you prefer, we can just make the error code sent to the client be the link, so like in that screenshot it would look more like so:
Method [...] does not exist Psalm(https://psalm.dev/022) [32, 57]
And it would still be clickable.

@pristinesource
Copy link
Contributor Author

Hmm, the CircleCI tests didn't actually show any tests that failed (some were skipped), but it says it failed 🤔

@weirdan
Copy link
Collaborator

weirdan commented Apr 16, 2020

Hmm, the CircleCI tests didn't actually show any tests that failed (some were skipped), but it says it failed thinking

It was killed while generating code coverage report, possibly by OOM killer:

Received "killed" signal

@muglug
Copy link
Collaborator

muglug commented Apr 16, 2020

we can just make the error code sent to the client be the link, so like in that screenshot it would look more like so

That sounds preferable – at some point the protocol will support links directly, but until then...

@pristinesource
Copy link
Contributor Author

@muglug ok, I will get that changed to be the link for both the value and the link. The protocol will support links directly in LSP 3.16.0 which is what I am implementing here. That is how it is going to be structured in the 3.16.0 specification 😉 .

@@ -383,7 +383,8 @@ function (IssueData $issue_data) use ($file_path) : Diagnostic {
'Psalm'
);

$code = 'PS' . \str_pad((string) $issue_data->shortcode, 3, "0", \STR_PAD_LEFT);
//$code = 'PS' . \str_pad((string) $issue_data->shortcode, 3, "0", \STR_PAD_LEFT);
$code = $issue_data->link;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@muglug Ok, the code now sent is the help link instead of the formatted short code.

@muglug muglug merged commit 6f36f33 into vimeo:master Apr 17, 2020
@muglug
Copy link
Collaborator

muglug commented Apr 17, 2020

Thanks, these are wonderful additions!

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.

None yet

3 participants