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

Add full support for Diagnostic.code #11765

Merged
merged 1 commit into from
Jan 13, 2023

Conversation

EstFoisy
Copy link
Contributor

@EstFoisy EstFoisy commented Oct 13, 2022

What it does

Closes #11659.
Add full support for Diagnostic.code according to the VS Code API.

How to test

CI should pass

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added the vscode issues related to VSCode compatibility label Oct 13, 2022
});
it('should convert a "code" of type "{ value: number, target: Uri }"', () => {
const uri = types.URI.parse('foo://example.com:8042/over/there?name=ferret#nose');
assert.strictEqual(Converter.convertCode({ value: 'string', target: uri }), 'string');
Copy link
Contributor

@colin-grant-work colin-grant-work Oct 13, 2022

Choose a reason for hiding this comment

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

It looks like there's loss of information here. Is there a reason we don't want to include the target field in the code when we convert it? I notice that the corresponding code in VSCode does preserve the target:

https://github.com/microsoft/vscode/blob/65a9097aa2da4d55318073ca23bc678a2885c461/src/vs/workbench/api/common/extHostTypeConverters.ts#L198-L207

Though when they're reconstructing a Diagonstic from the DTO, they ignore the target. Not very clear (to me) why.

https://github.com/microsoft/vscode/blob/65a9097aa2da4d55318073ca23bc678a2885c461/src/vs/workbench/api/common/extHostTypeConverters.ts#L223

Copy link
Member

Choose a reason for hiding this comment

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

@colin-grant-work I believe it was done this way due to the conversion of vst.Diagnostic (vscode-languageserver-types) which did not support the new type:

function reviveMarker(marker: MarkerData): vst.Diagnostic {
const monacoMarker: vst.Diagnostic = {
code: marker.code,
severity: reviveSeverity(marker.severity),
range: reviveRange(marker.startLineNumber, marker.startColumn, marker.endLineNumber, marker.endColumn),
message: marker.message,
source: marker.source,
relatedInformation: undefined
};

/**
 * Represents a diagnostic, such as a compiler error or warning. Diagnostic objects
 * are only valid in the scope of a resource.
 */
export interface Diagnostic {
    /**
     * The range at which the message applies
     */
    range: Range;
    /**
     * The diagnostic's severity. Can be omitted. If omitted it is up to the
     * client to interpret diagnostics as error, warning, info or hint.
     */
    severity?: DiagnosticSeverity;
    /**
     * The diagnostic's code, which usually appear in the user interface.
     */
    code?: number | string;
    /**
     * A human-readable string describing the source of this
     * diagnostic, e.g. 'typescript' or 'super lint'. It usually
     * appears in the user interface.
     */
    source?: string;
    /**
     * The diagnostic's message. It usually appears in the user interface
     */
    message: string;
    /**
     * Additional metadata about the diagnostic.
     */
    tags?: DiagnosticTag[];
    /**
     * An array of related diagnostic information, e.g. when symbol-names within
     * a scope collide all definitions can be marked via this property.
     */
    relatedInformation?: DiagnosticRelatedInformation[];
}

We could from the types-converter convert fully, and only when it comes time to reviveMarker we update to ensure the types match.

@JonasHelming
Copy link
Contributor

@EstFoisy Do you plan to look at the review comments?

@EstFoisy
Copy link
Contributor Author

Hi @JonasHelming ,
Many thanks for advising. I was working with Vince for this PR and, from my understanding of the discussion I had with him, everything was ok and ready for this PR. I will look into it, try to fix the code to the best of my understanding, and see with him when he come back from vacation if all is good or if there is any further adjustment needed.

@JonasHelming
Copy link
Contributor

@vince-fugnitto @EstFoisy : Maybe we could get this into the January release?

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@vince-fugnitto vince-fugnitto merged commit 029f7ba into eclipse-theia:master Jan 13, 2023
@vince-fugnitto vince-fugnitto added this to the 1.34.0 milestone Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vscode: add full support for Diagnostic.code
4 participants