FMCorz / mdk

Moodle Development Kit. A collection of tools meant to make developers' lives easier.
GNU General Public License v3.0
84 stars 47 forks source link

Add vscode script to generate jsconfig.json #211

Closed andrewnicols closed 1 year ago

andrewnicols commented 1 year ago

This patch adds a new script for vscode to generate a jsconfig.json file.

This file is described at https://code.visualstudio.com/docs/languages/jsconfig and can be used to configure vscode and other similar editors to understand our module loading.

andrewnicols commented 1 year ago

Originally proposed in MDL-75783. See the video in that issue for a demonstration.

andrewnicols commented 1 year ago

Here's a list of editors which support LSP, and by virtue, the jsconfig.json:

https://microsoft.github.io/language-server-protocol/implementors/tools/

FMCorz commented 1 year ago

Hey Andrew, what's your reasoning for now adding that to core? Wouldn't you consider the file to be supporting developers?

I imagine not requiring MDK would be helpful in this context for instance: https://github.dev/moodle/moodle

andrewnicols commented 1 year ago

Hi Fred,

The file itself shouldn't be in core because we have to whitelist each folder containing JS and map each plugin instance to the frankenstyle name. If we include the jsconfig.json in core then it isn't really possible to use it when working with plugins. That means it can either work through github.dev, or it can be useful for contrib developers.

We can include a generator in core (I wrote it as a grunt task), and I'm not opposed to doing so, but we typically don't often include such generators in core. Additionally it would only be a master change I suspect.

Including it in MDK and making it more widely available for others to use too is possibly the more flexible appraoach, but I'm open to suggestions.

FMCorz commented 1 year ago

Thanks Andrew!

Right, make sense. I saw it as a similar task to Grunt's ignorefiles, where it relies on the source tree, including 3rd party plugins. I certainly do not mind pulling that into MDK, but I feel that it's rightful place is in core, perhaps even if jsconfig.json is not shipped by default.

Happy for the patch to be merged, I'd just personally not fiddle with the .git/info/exclude, but that's just me.

andrewnicols commented 1 year ago

I'm happy to push it for core too, though I feel that it would be useful in MDK too for older versions and in the interim.

What are your concerns with the exclude file?

FMCorz commented 1 year ago

What are your concerns with the exclude file?

Just that it's magic and unexpected, not sure if we do that elsewhere in MDK.

FMCorz commented 1 year ago

@andrewnicols Did you remove the git exclude thingy? I see you've also left in plugin.py code for a bug addressed in #212.

Is there now a plan to put that in core as well? Do you think invoking npm directly could fail in some instances? MDK has not invoked npm from Python or PHP scripts previously.

andrewnicols commented 1 year ago

Thanks Fred,

That was accidentally committed - I was trying to help the guy in #212 track the issue down on the train at the time.

MDL-75783 landed today so this is now in core.

I don't think that invoking npx directly should cause any issues, and this is really only a stopgap to make people's lives easier.

It's up to you what we do with this PR. I think it has some value being available in MDK to make it easier for developers to call it.

Re the .git/info/exclude, given it's also in the .gitignore for 311, 400, and master I think it's appropriate to add it (and I think we should really add it in other situations like mdk plugin add.

Cheers

FMCorz commented 1 year ago

Thanks Andrew.

Indeed, there probably should be MDK APIs to write to the git exclude file, and monitor what's in there. Doing it from a PHP script is not the ideal MDK way as it should be baked in Python. In any case, scripts are what they are, quick & dirty operations that do not belong in MDK itself, so I think it's OK if it's loose and not standardised as the rest of the program.

Cheers!