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.

Sign up for GitHub

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

Jump to bottom

No one package should depend on @theia/json #6647

Merged
merged 1 commit into from
Jan 22, 2020
Merged

No one package should depend on @theia/json #6647

merged 1 commit into from
Jan 22, 2020

Conversation

tolusha
Copy link
Contributor

@tolusha tolusha commented Nov 27, 2019

Signed-off-by: Anatoliy Bazko [email protected]

What it does

PR removes dependency on @theia/json. It is allows to replace it with builtin extension.
https://www.npmjs.com/package/@theia/vscode-builtin-json-language-features

How to test

  1. Built Theia
  2. Open launch.json and preferences.json.
  3. Check if auto-completion works.

Review checklist

Reminder for reviewers

@tolusha tolusha added the json issues related to the json language label Nov 27, 2019
@tolusha tolusha self-assigned this Nov 27, 2019
@akosyakov
Copy link
Member

akosyakov commented Nov 27, 2019 •
edited
Loading

@tolusha I assume for testing @theia/json should be removed from the app and vscode built-in json extension installed and it still should work? Is it the goal?

@tolusha
Copy link
Contributor Author

tolusha commented Nov 27, 2019

@akosyakov
correct

@benoitf
Copy link
Contributor

benoitf commented Nov 27, 2019

linking #4217

@akosyakov akosyakov added the vscode issues related to VSCode compatibility label Nov 28, 2019
svenefftinge
svenefftinge approved these changes Dec 30, 2019
View reviewed changes
Copy link
Contributor

@svenefftinge svenefftinge left a comment

Choose a reason for hiding this comment

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

LGTM

@akosyakov akosyakov mentioned this pull request Jan 15, 2020
Merged
8 tasks
@vince-fugnitto
Copy link
Member

@tolusha do you mind fixing the conflicts and merging?
I'd like it to be included so I can use the json builtin (so we can get syntax highlighting once #6933 is merged)

@akosyakov
Copy link
Member

I'd like it to be included so I can use the json builtin (so we can get syntax highlighting once #6933 is merged)

@vince-fugnitto Did you test already whether it provides content assist properly for setting files?

@vince-fugnitto
Copy link
Member

I'd like it to be included so I can use the json builtin (so we can get syntax highlighting once #6933 is merged)

@vince-fugnitto Did you test already whether it provides content assist properly for setting files?

Unfortunately I haven’t gotten the chance to try out if it provides content assist but I suspect that it comes from the vscode-built in-json-language-features extension. I see a bug coming once #6933 is approved because the grammar for JSON is currently coming from @theia/textmate-grammars which is soon the be deprecated. I can confirm tomorrow when I test.

@akosyakov
Copy link
Member

Unfortunately I haven’t gotten the chance to try out if it provides content assist but I suspect that it comes from the vscode-built in-json-language-features extension. I see a bug coming once #6933 is approved because the grammar for JSON is currently coming from @theia/textmate-grammars which is soon the be deprecated. I can confirm tomorrow when I test.

I mean dynamic content assist, aware about schemas for our settings files and so on.

@tolusha
Copy link
Contributor Author

tolusha commented Jan 22, 2020

@vince-fugnitto
Hi, Sorry I don't have time, feel free to finish the task.

@vince-fugnitto
Copy link
Member

@vince-fugnitto
Hi, Sorry I don't have time, feel free to finish the task.

Sure! I updated the pull request fixing the conflicts while also referencing #4217.

@akosyakov
Copy link
Member

@vince-fugnitto please merge, maybe a work in CHANGELOG about adding @theia/json explicitly. I'm not sure about using built-in json extension, probably it will ignore all our schemas. We have to test it.

@vince-fugnitto
Copy link
Member

@vince-fugnitto please merge, maybe a work in CHANGELOG about adding @theia/json explicitly. I'm not sure about using built-in json extension, probably it will ignore all our schemas. We have to test it.

Sure, I'll add an entry for the changelog.

@tolusha @vince-fugnitto
[json] remove dependencies to @theia/json
5413de0 
Fixes #4217

Removes dependencies to `@theia/json` from both the
`@theia/preferences` and `@theia/debug` extensions so that
we can use vscode builtin extensions instead.

Signed-off-by: Anatoliy Bazko <[email protected]>
@vince-fugnitto vince-fugnitto merged commit a4a09fc into master Jan 22, 2020
@vince-fugnitto vince-fugnitto deleted the ab/fixjson branch January 22, 2020 20:29
@akosyakov akosyakov mentioned this pull request Apr 22, 2020
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto left review comments

@benoitf benoitf benoitf left review comments

@svenefftinge svenefftinge svenefftinge approved these changes

@paul-marechal paul-marechal Awaiting requested review from paul-marechal

@thegecko thegecko Awaiting requested review from thegecko

Assignees

@tolusha tolusha

Labels
json issues related to the json language vscode issues related to VSCode compatibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tolusha @akosyakov @benoitf @vince-fugnitto @svenefftinge