PhilippeChab / nwscript-ee-language-server

A NWScript Language Server.
https://marketplace.visualstudio.com/items?itemName=PhilippeChab.nwscript-ee-language-server
MIT License
20 stars 7 forks source link

allow user to define a source path for NWN installation folder #27

Closed tinygiant98 closed 2 years ago

tinygiant98 commented 2 years ago

I added some functionality to allow the user to specificy the installation path for their NWN installation, so they don't have to copy and paste and nwscript.nss file into the server resources folder. It should look something like this:

yarn run generate-lib-defs --path "C:/Path/To/Neverwinter Nights"

The path argument is the path (absolute or relative should work) to the game installation, not to the nwscript.nss file. The code will add /ovr/nwscript.nss to whatever path is provided. This has only been tested on window. If no path is specified, original behavior (looking for nwscript.nss in the server resources folder) is used. On Windows, any spaces in the quoted path argument must be prefixed with a backtick, but that might be a terminal-specific implementation. Only test on powershell.

I don't know much about typescript, so I haven't been able to find a reliable way to deliver the resulting file to the extension installation directory since the folder name includes a version number.

PhilippeChab commented 2 years ago

Hey! Thank you for the contribution!

At first I thought I liked the proposition, however after a second thought I am not sure anymore and here's why:

  1. If we update the json file by providing the path parameter but forget to update nwscript.nss in the /resources directory, someone could run the command without the path parameter later on and generate the json file with the wrong version of nwscript.nss. By enforcing the command to run only with the nwscript.nss inside the project, we ensure it is always properly updated.
  2. Having the nwscript.nss file next to its complex tokens json file allow us to validate that the tokenization process is not broken if we make changes to it, since we are sure the json file is always updated using the same source file. (By not broken I mean that we can ensure the result is what we expect it to be.)

What do you think?

tinygiant98 commented 2 years ago

Thanks for reading my PR!

There's two ways to look at it here. One is that if the --path argument was used on first use, the nwscript.nss file would never have existed in the /resources directory to begin with (as it's not supplied with the lsp, which it shouldn't be). Therefore, a later run wouldn't have any conflict with a previous run. In fact, a later run would fail because there is no nwscript.nss file in the /resources directory to source from. The other way is, maybe assuming semi-advanced usage, if the person is capable enough to install the lsp without the extension being generally available on the marketplace (i.e. they build it from your repo), they should be competent enough to understand what file is being used and what results they are getting. Of course, once (if?) this goes to general release and is available through the marketplace, that last assumption may be faulty.

But to prevent some or all of this, maybe a better option would be to allow the user to supply a path in their vscode settings file? Allowing the user to use the nwscript that resides in the installation directory ensures they're always using the most up to date version. One of the issues with a copied file (specifically nwscript.nss) is that there's no way to know which game version it belongs unless you're familiar with various commands that were implemented in each version. So even an outdated nwscript.nss in the /resources folder may appear to be current.

PhilippeChab commented 2 years ago

I do agree with the following point:

One of the issues with a copied file (specifically nwscript.nss) is that there's no way to know which game version it belongs unless you're familiar with various commands that were implemented in each version. So even an outdated nwscript.nss in the /resources folder may appear to be current.

Although we can consider that the version of the file is always the latest.

This I believe is however not a good idea:

But to prevent some or all of this, maybe a better option would be to allow the user to supply a path in their vscode settings file?

The extension settings should be kept for its use, not for its development.

We can add the --path parameter for an ease of development. It will be easy to spot an update to standardLibDefinitions.json without nwscript.nss being updated as well in a PR, so we will be able to guard against such mistake.

I would do the three following changes:

  1. Rename path to source - or something similar. path is confusing as it could be understood as the destination path for the generated file.
  2. Remove the ovr concatenation. We should not assume the user points to its Neverwinter Nights installation folder, it could be anywhere.
  3. Could you add documentation to the README regarding the new argument? :)

Thank you!

tinygiant98 commented 2 years ago

Yup, I'll look at all that and see what I can do. Thanks!