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

Allow using a project-level catalog file as well #3

Merged
merged 6 commits into from
Apr 7, 2023

Conversation

greystate
Copy link
Contributor

This is a draft PR, just in case - curious to hear if you'd consider adding this feature.

This looks in the project's configuration for a setting called xml.catalogFolder and if found, will try to load a catalog.xml file from that location and add it to the initialization options' catalogs array.

I'm not sure if the filename should be configurable as well - maybe it's simpler to name the setting xml.catalogFile instead, and expect the full path?

Cheers,
Chriztian

Look for the configuration setting `xml.catalogFolder` and load `catalog.xml` file from there if specified.
@robb-j
Copy link
Owner

robb-j commented Mar 31, 2022

Thanks for the PR! This could definitely be added. I think it would make sense to specify the a file rather than a folder so you aren't locked into calling it "catalog.xml", unless this is a standard XML thing I'm not aware of?

You can also add a configWorkspace option to the extension.json too. This would allow people to configure it though the GUI as well.

I wonder if it makes sense to allow multiple catalogs per project rather than just one? If so it could be a pathArray configuration type that would be an array instead of a single string. It would be easier to decide this now to try to avoid future breaking changes.

@greystate
Copy link
Contributor Author

Oh very nice - I didn't even think to see if a workspace level config was possible! I was really hoping for a GUI option of some kind. I'll try this out right away.
Makes total sense to go with files in a pathArray - hopefully I can translate that to the Type-/JavaScript as well :)

@greystate
Copy link
Contributor Author

greystate commented Mar 31, 2022

Works so much better than having to manually edit the Configuration.json file. Also gets rid of all the messy string/path manipulations 👍

Only thing that's a bit wonky is that when you open a project the first time and add a catalog file, you'll have to close any XML file that would use it and open it again to get the correct validations. I will check if it's possible to do a "watch" or similar on the config... and then somehow re-trigger any validations.

Update: Looks like onDidChange(key, callback) can be used for this...

@robb-j
Copy link
Owner

robb-j commented Mar 31, 2022

The update might actually be related to this nova 9.x issue — https://devforum.nova.app/t/regression-in-novas-lsp-configuration-support/1460/4

normally nova automatically syncs the project and global config from your with the language server but there is a regression there. It might not be worth looking too far into it until that is resolved, hopefully in nova 9.2.

edit - actually no, I'm being silly this is an initialisation parameter not a language server configuration, so we do need to look into a way to resend the custom catalog files

@robb-j
Copy link
Owner

robb-j commented Mar 31, 2022

It might be possible to use nova's config sync (when it is working again) to get this all working more automatically. If the config is named simply "xml.catalogs" it should line up with the language sever's configuration and nova will automatically tell the server about the value and update it when it changes. If we went this route then the initialisationOptions should be able to stay as it was and this config might automatically work.

@robb-j
Copy link
Owner

robb-j commented Jul 3, 2022

Nova's configuration API is still broken 😞 https://devforum.nova.app/t/regression-in-novas-lsp-configuration-support/1460/15

I can merge this in now if you like, then I can update to use the configuration API when/if that gets fixed? Let me know what you think.

@greystate
Copy link
Contributor Author

That sounds great Robb - I've only been following along sporadically so wasn't really aware.

I'll convert this to a proper PR then and you can go ahead and merge it when you've confirmed it doesn't blow up your otherwise lovely extension 😁

Thanks!

@greystate greystate marked this pull request as ready for review July 9, 2022 07:39
@robb-j
Copy link
Owner

robb-j commented Feb 20, 2023

Hey, I missed that you marked this as ready for review sorry! I'll look into this soon, apologies

@greystate
Copy link
Contributor Author

No worries Rob 👍 (Though I no longer remember if I've verified that this actually works 😕 )

@robb-j
Copy link
Owner

robb-j commented Mar 11, 2023

Hey, it looks like I was already testing setting "xml.catalog" from Nova's LanguageServer config integration and it seems to be working! I've just released 0.3.0 which contains this if you could try it out?

It doesn't need the xml-language-server.ts or extension.json changes now but but I'd still like to merge your doc changes, then you'd get properly credited GitHub wise!

@greystate
Copy link
Contributor Author

Hi @robb-j

Thanks for the update! I've tried to see if I could make it work, but haven't yet succeeded in any of my projects, so I've created a very minimal test project that I hope you can try out and see if I'm just making things up 😅

It's located here: https://github.com/greystate/xml-reduced-case

When I open the "wordles.xml" file in Nova, I just get the "No grammar constraints (DTD or XML Schema)" message...

I've tried both having the catalog point to the "real" location (https://xmlns.greystate.dk/2022/wordles/wordles.xsd) and to the local copy that's included in the repo.

If I open the wordles.xml file in Xmplify.app (native macOS XML editor) and add the catalog file, it works "correctly", i.e. it understands the schema and provides help etc.

Hope I'm making sense :)

/Chriztian

@robb-j robb-j merged commit 55b3a1c into robb-j:main Apr 7, 2023
@robb-j
Copy link
Owner

robb-j commented Apr 7, 2023

Hey, I know I merged it but its taking a bit of investigation to get it working. Looking at your xsd:annotation's I think they aren't working because the LangaugeServer doesn't support that HTML-in-XML syntax? I'm by no means an expert on XSD, when I wrote the schema for nova syntaxes the LanguageServer treated the text inside there as markdown? I hacked around your file and putting plain text did get the hovers working!

Screenshot 2023-04-08 at 00 26 42

<xsd:annotation>
  <xsd:documentation>
    One five letter word as one of 6 tries at solving the wordle (or 9 tries in a quordle).
  </xsd:documentation>
</xsd:annotation>

@greystate
Copy link
Contributor Author

Cool @robb-j 👍 - I can totally work with that (Markdown, that is...)

I'll do some testing in my files - thanks!!

@robb-j
Copy link
Owner

robb-j commented Apr 9, 2023

The catalogs aren’t working at the moment, something in Nova or the LanguageServer changed and it’s not behaving well. I’m working on that over in #8, I should hopefully have a release for that soon.

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.

None yet

2 participants