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

[textmate] don't warn when same grammar is registered multiple times #6125

Merged
merged 1 commit into from
Sep 6, 2019

Conversation

svenefftinge
Copy link
Contributor

Fixes #6124

What it does

No more warnings when the same grammar is registered multiple times.

How to test

Remove textmate-grammars and install https://registry.npmjs.org/@theia/vscode-builtin-ini/-/vscode-builtin-ini-0.2.1.tgz in plugins folder

See how no warning is logged (if you do the same without this change there will be warning)

Review checklist

Reminder for reviewers

@svenefftinge svenefftinge requested a review from a team as a code owner September 6, 2019 08:05
const existingProvider = this.scopeToProvider.get(scope);
if (existingProvider) {
Promise.all([existingProvider.getGrammarDefinition(), description.getGrammarDefinition()]).then(([a, b]) => {
if (a.location !== b.location) {
Copy link
Member

Choose a reason for hiding this comment

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

only if both locations are not undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think that's enough. But if you insist ... :-)

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

tested that waning is reported only when locations are different, but warning should be also reported if locations are undefined

@akosyakov akosyakov added textmate issues related to the textmate grammars vscode issues related to VSCode compatibility labels Sep 6, 2019
console.warn(new Error(`a registered grammar provider for '${scope}' scope is overridden`));
}
});
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm removing this return again.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

it looks good to me now

@svenefftinge svenefftinge merged commit e2d6355 into master Sep 6, 2019
@svenefftinge svenefftinge deleted the GH-6124 branch September 6, 2019 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
textmate issues related to the textmate grammars vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[textmate] Don't warn if same grammar gets configured multiple times
2 participants