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

Proposal: Add an API to expose setting configs #292

Merged
merged 1 commit into from
Jul 27, 2020

Conversation

a2975667
Copy link
Contributor

Hello Redhat-developer team,
I'm an intern with Salesforce working to support Salesforce developers on XML files. We thought we could make use of the Redhat XML plugin to accomplish this function. We utilize the XML catalog settings and the XML fileAssociation settings to achieve this. However, we do not want to write directly into the developer's settings.json (even on a workspace level) and hope to accomplish feeding the extension of these settings by calling the extension API.

Here we'd like to make a PR and expose these two APIs that Salesforce extensions could call
setXMLCatalog() and setXMLFileAssociation() by passing in a JSON from inside Salesforce's extension.
One challenge we had here is because, in the activation function, the getXMLSettings will always source directly from the settings.json file. This behavior means that every time getXMLSettings gets called, our registered settings will be lost. Is there any advice on how to resolve this, or is there another design that the RedHat-develop team suggests?

src/extension.ts Outdated Show resolved Hide resolved
@angelozerr
Copy link
Contributor

Hi @a2975667

Thanks a lot for your contribution. We are totally aware to help external component to contribute to vscode-xml, LemMinx features, but we are careful when we need to provide new API. Wee need to provide the proper API which could be used for a lot of usecases and not for one usecase.

I'm not against your contribution, but I would like to know how it will work with your Salesforce context, because I wonder if it should be better that you write a Java LemMinx extension to manage your usecase. Developping a LemMinx extension will provide the feature for another IDE/Editor like Eclipse.

Could you explain us more what you want to do. I read that Salesforce have some meta package XML files like:

<?xml version="1.0" encoding="UTF-8"?>
<Package xmlns="http://soap.sforce.com/2006/04/metadata">
    <types>
        <members>xyzsite</members>
        <name>SiteDotCom</name>
    </types>
    <version>30.0</version>
</Package>

Is this kind of files you want to manage? I suppose you have XML catalog to bind http://soap.sforce.com/2006/04/metadata namespace with a XML Schema? Perhaps a LemMinx extension could be enough without using XML catalog. With LemMinx extension you could manage custom completion with Java for members values for instance (if XML Schema doesn't manage enum).

In other words please explain in details your usecase in order to find the best solution for your usecase and see if we need your contribution or not.

@ntotten
Copy link

ntotten commented Jul 17, 2020

The use case you are mentioning is exactly what we are trying to solve, code completion for all of our XML metadata - of which there are many.

We did look into building something with LemMinx as you mentioned, but there were really two things that turned us away from that. The first is that (almost) all of our tooling is written in Typescript/Javascript. It's not that we couldn't provide this with Java, we certainly have Java devs, but the barrier and maintenance of that will be significantly higher for our team.

Second, it seemed like writing a whole new LemMinx extension was significantly more effort than just providing references to XSD files. Right now, we have a very similar process for adding schema validation for our JSON files with VS Code's built-in JSON language server. You can see that adding the schemas is really just a simple file pattern reference: https://github.com/forcedotcom/salesforcedx-vscode/blob/develop/packages/salesforcedx-vscode-core/package.json#L813-L822 That is really the type of experience we were hoping for with XML as well.

I'm certainly open to discussion, but my feeling here is that this type of API would be very popular with VS Code extension developers - and while I understand it means it isn't cross-IDE compatible, I think that it still adds a lot of value to those using VS Code so the tradeoff is justified.

@angelozerr
Copy link
Contributor

We did look into building something with LemMinx as you mentioned, but there were really two things that turned us away from that. The first is that (almost) all of our tooling is written in Typescript/Javascript. It's not that we couldn't provide this with Java, we certainly have Java devs, but the barrier and maintenance of that will be significantly higher for our team.

Ok I understand what you mean, but do you think you wanted to provide custom completion (when XML Schema is not enough and values are dynamic (like goal of pom.xml)) ? If it is the case, I think Java dev will be required.

You can see that adding the schemas is really just a simple file pattern reference: https://github.com/forcedotcom/salesforcedx-vscode/blob/develop/packages/salesforcedx-vscode-core/package.json#L813-L822 That is really the type of experience we were hoping for with XML as well.

Indeed I love this JSON feature which is so powerfull and simple. Why not having this same feature? I mean instead of declaring XML catalog, file associations in a settings.json, we could declare it in the package.json. When the XML Language server is started, the settings sent to the server will be the result of merge from package.json and user settings. What do you think about this idea? @fbricon what do you think?

The only thing to do on LemMinx side is to distinguish user settings from static settings which cannot be changed (coming from package.json in vscode context).

src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
@fbricon
Copy link
Collaborator

fbricon commented Jul 17, 2020

I think you need to keep track of the contributed settings somewhere, then merge them with the regular settings whenever those regular settings change, so they can be sent to the server with

languageClient.sendNotification(DidChangeConfigurationNotification.type, { settings: getXMLSettings(requirements.java_home) });

@angelozerr
Copy link
Contributor

angelozerr commented Jul 17, 2020

@a2975667 @ntotten what about declaring file association and XML catalog in package.json like JSON does?

IMHO I think it should be a better idea to do that for you to be consistent with JSON.

I would prefer having this feature instead of exposing a new API (if @fbricon is OK for that)

I suggest that you see https://github.com/microsoft/vscode/blob/ff6b1c5984a4c2116a06318f07e871a4169fdb1b/extensions/json-language-features/client/src/jsonClient.ts#L356

@fbricon
Copy link
Collaborator

fbricon commented Jul 17, 2020

contributing file associations via package.json config is indeed preferable, if possible, since we wouldn't need to expose/maintain a new API.
The new API would be useful for dynamic registration though. It all depends on @ntotten @a2975667 needs. No need to get ahead of ourselves if we don't need it ;-)

@ntotten
Copy link

ntotten commented Jul 17, 2020

Ok I understand what you mean, but do you think you wanted to provide custom completion (when XML Schema is not enough and values are dynamic (like goal of pom.xml)) ? If it is the case, I think Java dev will be required.

It's certainly possible in the future but it really isn't something on our radar. I can maybe think of a few scenarios where we could provide richer completion (i.e. things that are specific for a particular instance or something), but almost all of the completions are standard from static XSDs.

Indeed I love this JSON feature which is so powerfull and simple. Why not having this same feature? I mean instead of declaring XML catalog, file associations in a settings.json, we could declare it in the package.json. When the XML Language server is started, the settings sent to the server will be the result of merge from package.json and user settings. What do you think about this idea? @fbricon what do you think?

I love the idea of supporting both. I think though for us putting everything in the package.json wont be practical since there are going to be hundreds of different schemas. I think we would want a way to register them dynamically. This actually matches what MS has done with the HTML language server where you can contribute web components for code completion. They allow registering in settings.json, through the extension contribution point in package.json or through the API. https://code.visualstudio.com/blogs/2020/02/24/custom-data-format

@angelozerr
Copy link
Contributor

I love the idea of supporting both.

Great.

I think though for us putting everything in the package.json wont be practical since there are going to be hundreds of different schemas. I think we would want a way to register them dynamically.

Ok thank's for your clarification, I understand more now why you need this dynamic feature.

src/extension.ts Outdated Show resolved Hide resolved
@angelozerr
Copy link
Contributor

src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
@angelozerr
Copy link
Contributor

@a2975667 please click on Resolve Conversation once you have fixed a problem, it will help more for the review, thanks!

Once you have fixed my last comment #292 (comment) and if it works for you (even with your usecase whith lost of settings), I approve this PR.

I let @fbricon to merge it if he think it's good.

@a2975667
Copy link
Contributor Author

Please let me know if there are any other things that I should address. Thanks! :)

@a2975667
Copy link
Contributor Author

@fbricon are there any other things that I miss addressing? Thanks so much again!

@fbricon
Copy link
Collaborator

fbricon commented Jul 22, 2020

Please squash your commits into one and use git commit -s to sign it

@a2975667
Copy link
Contributor Author

@angelozerr @fbricon @ntotten
Added an additional API to return the status of the language client.
This addition is helpful because there is, to the best of my knowledge, a way to know if the extension's language server is ready or not. The current .isActive() seems to only checks if activate() was invoked.
Having this additional API makes sure that the extension is ready and one can safely apply the API calls.
Please comment.

@fbricon
Copy link
Collaborator

fbricon commented Jul 24, 2020

@a2975667 can you please sign-off both your commits? (see https://git-scm.com/docs/git-commit#Documentation/git-commit.txt---signoff)

@a2975667
Copy link
Contributor Author

@fbricon Just did. Sorry for that.

@a2975667
Copy link
Contributor Author

@fbricon Please let me know if there are any other things I need to address. Thanks!

@fbricon fbricon merged commit c6c32dc into redhat-developer:master Jul 27, 2020
@fbricon
Copy link
Collaborator

fbricon commented Jul 27, 2020

Thanks for your contribution @a2975667 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants