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

use ReadonlyArray for DocumentSelector #10070

Merged
merged 8 commits into from
Sep 14, 2021
Merged

Conversation

smart-bo
Copy link
Contributor

@smart-bo smart-bo commented Sep 8, 2021

What it does

Change Array<DocumentFilter> to ReadonlyArray<DocumentFilter> at: theia/packages/plugin/src/theia.d.ts

Fix #10025

How to test

N/A

Review checklist

Reminder for reviewers

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.

@smart-bo thank you for the pull-request 👍

Please be sure to properly sign the eclipse contributor agreement (eca) with the same email as your authorship so we can accept your changes.

@vince-fugnitto vince-fugnitto added the vscode issues related to VSCode compatibility label Sep 8, 2021
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.

The code looks good to me 👍

Copy link
Contributor

@tsmaeder tsmaeder left a comment

Choose a reason for hiding this comment

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

@smart-bo thx for the contribution. I have a couple of nitpicks.

packages/plugin-ext/src/plugin/languages.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/plugin/type-converters.ts Outdated Show resolved Hide resolved
({
range: fromRange(r)!
}));
({
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the formatting the typescript formatter creates. Let's stick to that unless there is a compelling reason.

Copy link
Member

Choose a reason for hiding this comment

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

Do you have directions on how to set this up? VS Code formats like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@paul-marechal it doesn't for me 🤷

@tsmaeder tsmaeder merged commit 8df74a7 into eclipse-theia:master Sep 14, 2021
@smart-bo smart-bo deleted the gh10025 branch September 14, 2021 13:28
@msujew msujew mentioned this pull request Sep 15, 2021
1 task
RomanNikitenko pushed a commit that referenced this pull request Sep 16, 2021
RomanNikitenko pushed a commit that referenced this pull request Sep 16, 2021
@smart-bo smart-bo mentioned this pull request Sep 21, 2021
1 task
@vince-fugnitto vince-fugnitto added this to the 1.18.0 milestone Sep 30, 2021
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.

DocumentSelector Should use read-only array of DocumentFilter
4 participants