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

Refactors calls to languages.setLanguageConfiguration to declarative descriptions in language-configuration.json #124015

Merged
merged 3 commits into from
Jun 8, 2021

Conversation

hediet
Copy link
Member

@hediet hediet commented May 17, 2021

This fixes #98621.

Edit from @alexdima: Since a couple of milestones, it is possible to express everything in the language configuration files, including the wordPattern and the onEnterRules. It is preferable to express the language configuration in JSON because this allows those features to work even when the main extension is not activated.

…descriptions in language-configuration.json. This fixes #98621.
@hediet hediet requested a review from alexdima May 17, 2021 07:13
@hediet hediet self-assigned this May 17, 2021
@alexdima alexdima added this to the May 2021 milestone May 18, 2021
Copy link
Contributor

@aeschli aeschli left a comment

Choose a reason for hiding this comment

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

As mentioned, I would remove the g from the wordPatterns in the language-configuration. Other than that. it all looks good!

],
"wordPattern": {
"pattern": "(-?\\d*\\.\\d\\w*)|([^\\`\\~\\!\\@\\$\\^\\&\\*\\(\\)\\=\\+\\[\\{\\]\\}\\\\\\|\\;\\:\\'\\\"\\,\\.\\<\\>\\/\\s]+)",
"flags": "g"
Copy link
Contributor

@aeschli aeschli May 18, 2021

Choose a reason for hiding this comment

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

I believe the 'g' is mandatory for all word patterns. I has to do with the way we use the word pattern regexes.
So I would remove it from the language-configurations and always set it where we create the RegExp object.

So the extension can do "wordPattern": "(-?\\d..." * (unless i or u is needed as well)

Copy link
Collaborator

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

Markdown changes look good. Should TypeScript also be updated?

const jsxTagsLanguageConfiguration: vscode.LanguageConfiguration = {

@hediet
Copy link
Member Author

hediet commented May 31, 2021

Markdown changes look good. Should TypeScript also be updated?

const jsxTagsLanguageConfiguration: vscode.LanguageConfiguration = {

I guess yes, but lets do that in another PR, otherwise this becomes a Frankenstein!
The typescript-basics language feature currently does not have separate language-configurations for ts and for tsx, however, the typescript-language-features registers different configurations for them.

Also, lets merge this after the May release so there is plenty of time to test these changes in the Insiders. I'm quite sure that my refactoring is correct, but it is very easy to screw up and copy the wrong thing.

@hediet hediet modified the milestones: May 2021, June 2021 May 31, 2021
@hediet hediet merged commit ce5023b into main Jun 8, 2021
@hediet hediet deleted the hediet/refactor-language-configuration branch June 8, 2021 14:19
@github-actions github-actions bot locked and limited conversation to collaborators Jul 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Activate embedded languages for indent rules
5 participants