Try to get contextive definitions if not defined in the config

[erikjuhani]

Hello @chrissimon-au! Firstly thanks for the great concept (and the language-server that comes with it!).

Background

This concept sounds really useful, as the knowledge bank can be stored in one file and work across different file formats. I can see multiple use cases for this. As an avid user of neovim, I was eager to try this language server with my setup. Unfortunately, probably due to my lack of knowledge in the realm of LSP (Language Server Protocol), I couldn't get it to work right out of the box. I kept encountering the missing contextive.path error. As a result, I decided to make the user experience a bit easier, meaning less necessary configuration, so that "it just works".

Neovim setup

I'm using the neovim builtin lsp with lspconfig, and this is my working setup for contextive language-server:

local ok, configs = pcall(require, "lspconfig.configs")
if not ok then
  return
end

configs.contextive = {
  default_config = {
    filetypes = { "*" },
    on_attach = on_attach,
    root_dir = lspconfig.util.root_pattern('.contextive'),
    cmd = { "~/lsp/Contextive.LanguageServer" }, -- This is a temporary path for now
  },
}

lspconfig.contextive.setup {}

The above language-server configuration is attached to all buffers / all filetypes.

Implementation

This is an enhancement to the language server that enables it to automatically use the contextive definitions path if it's found in the workspace, and if the contextive.path configuration is not defined.

Add a new function called getContextiveDefinitionsPath, which returns the contextive definitions path if it exists in the workspace.

By default, the language-server uses the contextive.path defined by the caller.

And this is how it works and looks like in the sample repository:

[chrissimon-au]

Hi @erikjuhani, thanks so much for this contribution! You have the honour of being the first community code contribution to Contextive 😍!

Unfortunately this means you are somewhat paving the path with me here - as I haven't yet properly setup community contribution guidelines or any automated pr checks - so I appreciate your support & patience as we work out together how to get this improvement into a release :)

Also, well done working out this solution given the limited developer documentation!

Tests

The good news is that although there are no pr checks setup yet, I have cloned your fork and run the tests locally and I'm happy that all existing tests are still passing.

However, I have been using TDD to build contextive, and while I won't insist contributors use TDD, I would like to ensure all meaningful features/scenarios are represented in a test.

In this case, I think a new test case in src/language-server/Contextive.LanguageServer.Tests/InitializationTests.fs would be helpful - if you review those tests, they configure the OmniSharp language client for a variety of scenarios including various combinations of workspace and setting value. But there is no scenario yet for where the setting value just doesn't exist.

Could you please take a look at that and see if you can work out how to construct an equivalent situation in that test as to how the neovim lsp client will present to the server? Ideally the test should fail with the current official release and pass with your release. If it's not clear let me know and we can setup a pairing session to work through it together.

Design

It seems that this change makes contextive language server more resilient if the client does not provide the contextive.path configuration setting. In VsCode (which has been my focus so far), the setting is defined in the extension - with a default value. So the client has always provided a value, either this default or the user configured value.

Now that I see how neovim is likely to work it makes me realise that the default value should be defined in the server (as you have done) but now we have a potential conflict between the server default and the vscode extension default. I will take a look at the vscode extension to see how best to ensure that the new server default is used consistently.

How would this neovim configuration work if the repository had the definitions file NOT in the default location? In vscode there is a settings.json file that overrides the default config value for the path. This is helpful as it's in the repository so all devs on the team get the same setting. Is there a way to do something similar with neovim?

Official Neovim support

Do you have a recommendation on how we can simplify life for other neovim users?

A few questions come to mind:

1. Distributing the language server in isolation (how did you obtain it?) - I guess an extra release artifact in the github release would be a good start?
2. Guidance on configuring neovim (we could add a README in a neovim specific folder in the repo and link to it from the root README?
3. Should we consider contributing a PR to add a default contextive config to the https://github.com/neovim/nvim-lspconfig project?

Thanks!

Thanks again, look forward to your thoughts on the above :)

[erikjuhani]

Thanks @chrissimon-au for the detailed answer! I hope I didn't split my answers too much, but I wanted to keep the context small.

Hi @erikjuhani, thanks so much for this contribution! You have the honour of being the first community code contribution to Contextive 😍!

You're welcome! I'm delighted to be the first community contributer to Contextive. 😊

Unfortunately this means you are somewhat paving the path with me here - as I haven't yet properly setup community contribution guidelines or any automated pr checks - so I appreciate your support & patience as we work out together how to get this improvement into a release :)

I understand! I hope I can help you to setup the requirements and expectations. I agree that PR checks would be in order—firstly to ease the maintainer burden, but also give more confidence to the contributor itself.

Also, well done working out this solution given the limited developer documentation!

Thanks! It was quite easy to setup. Probably due to dotnet tooling and fsharp. Although this was my first time ever writing fsharp. 😀

Tests

The good news is that although there are no pr checks setup yet, I have cloned your fork and run the tests locally and I'm happy that all existing tests are still passing.

In hindsight, I should have mentioned that I ran all the tests, and they passed before opening this PR (sorry about that 🙁). I also conducted manual testing with my Neovim setup. Also I think it would be great to provide specific steps for setting up the development environment for the Contextive VSCode extension, as that is something I did not test locally.

However, I have been using TDD to build contextive, and while I won't insist contributors use TDD, I would like to ensure all meaningful features/scenarios are represented in a test.

Perhaps this is something worthwhile to mention in the upcoming contribution guidelines! 😉
But I agree it's a great practice to have, especially if the tests provide more confidence.

In this case, I think a new test case in src/language-server/Contextive.LanguageServer.Tests/InitializationTests.fs would be helpful - if you review those tests, they configure the OmniSharp language client for a variety of scenarios including various combinations of workspace and setting value. But there is no scenario yet for where the setting value just doesn't exist.

Could you please take a look at that and see if you can work out how to construct an equivalent situation in that test as to how the neovim lsp client will present to the server? Ideally the test should fail with the current official release and pass with your release. If it's not clear let me know and we can setup a pairing session to work through it together.

I'll take a look at those tests and add this particular scenario, thank you for pointing this out!

Design

It seems that this change makes contextive language server more resilient if the client does not provide the contextive.path configuration setting. In VsCode (which has been my focus so far), the setting is defined in the extension - with a default value. So the client has always provided a value, either this default or the user configured value.

Yes that was my idea, to make the Contextive language-server easier to use, so it would work with less configuration. For example I just couldn't get the contextive.path to work, but that's probably an error from my side as I'm not that fluent with setting up a custom LSP or even working with LSP specific problems.

Now that I see how neovim is likely to work it makes me realise that the default value should be defined in the server (as you have done) but now we have a potential conflict between the server default and the vscode extension default. I will take a look at the vscode extension to see how best to ensure that the new server default is used consistently.

That is indeed true. I'd propose that the language server acts as the source of truth and the extension by default should use the default from the language server instead. That way we centralize the logic, which affects all the clients that use the Contextive language server.

How would this neovim configuration work if the repository had the definitions file NOT in the default location? In vscode there is a settings.json file that overrides the default config value for the path. This is helpful as it's in the repository so all devs on the team get the same setting. Is there a way to do something similar with neovim?

In neovim there's a separate server settings that can be applied, as an example my gopls setup:

lspconfig.gopls.setup {
  on_attach = on_attach,
  capabilities = capabilities,
  cmd = { "gopls", "serve" },
  settings = {
    gopls = {
      analyses = {
        unusedparams = true,
      },
      staticcheck = true,
      linksInHover = false,
      codelenses = {
        generate = true,
        gc_details = false,
        regenerate_cgo = true,
        tidy = false,
        upgrade_depdendency = true,
        vendor = true,
      },
      usePlaceholders = true,
    },
  },
}

For some reason I couldn't get this particular setting to work for Contextive, specifically the contextive.path.

In my mind, in order for this to work with other editors. Contextive would need to provide a separate runtime configuration file (.contextiverc or something similar) or just use a default location in the repository like the .contextive/definitions.yml, and not couple the settings with editor specific configuration file.

As far as I know there is no similar option for neovim out of the box like the settings.json in vscode.

Official Neovim support

Do you have a recommendation on how we can simplify life for other neovim users?

A few questions come to mind:

1. Distributing the language server in isolation (how did you obtain it?) - I guess an extra release artifact in the github release would be a good start?

I obtained the language-server by building it on my own machine, as there's no artifacts available currently, and yeah I'd say having the github release artifacts for different operating systems and architectures would be a good starting point! After that I would consider including the language server in different package managers like brew, apt, nix, etc.

2. Guidance on configuring neovim (we could add a README in a neovim specific folder in the repo and link to it from the root README?

3. Should we consider contributing a PR to add a default contextive config to the https://github.com/neovim/nvim-lspconfig project?

I would approach this in a tool agnostic way! So the setup could serve multiple different editors. I guess there could be some mention or example configuration for neovim, but maybe a focus for now could be to provide a way to easily setup the Contextive language server locally by building it youself and adding it to system path for example.

Definitely I'd consider a contribution to nvim-lspconfig and also (mason-registry)[https://github.com/mason-org/mason-registry], which would make it a breeze to install in neovim, but before those contributions I think the language-server should be available as a separate artifact in the github releases! Example configuration from the mason registry: https://github.com/mason-org/mason-registry/blob/main/packages/arduino-language-server/package.yaml

[chrissimon-au]

Hi @erikjuhani thanks for the thorough response! Based on this conversation I've setup the following issues:

• Move contextive.path default to Contextive.LanguageServer for vscode extension #51
• Release Language Server as independent Component #50
• Support vim #33 (updated to define a few steps in providing official support)

I'll be taking a look at #50 later this week, but will wait until #49 is merged before doing #51. Then, if you're open to it, your assistance with #33 would be appreciated!

Let me know how you go with adding tests, look forward to getting this merged :)

[erikjuhani]

Hi @erikjuhani thanks for the thorough response! Based on this conversation I've setup the following issues:

• Move contextive.path default to Contextive.LanguageServer for vscode extension #51
• Release Language Server as independent Component #50
• Support vim #33 (updated to define a few steps in providing official support)

I'll be taking a look at #50 later this week, but will wait until #49 is merged before doing #51. Then, if you're open to it, your assistance with #33 would be appreciated!

Let me know how you go with adding tests, look forward to getting this merged :)

Hey! Sorry there was a bit of delay in my answer. I did try to add these particular test cases however:

• When workspace and contextive definitions file are not found
• When workspace is defined, but contextive definitions file is not found
• When workspace is defined and contextive definitions file is found

Unfortunately I can’t wrap my head around how those tests should be implemented, due to current ones being quite complex and my knowledge of dotnet and fsharp limited. I tried to make a proper end to end test for initializing by setting up a temporary path and definitions file. That however broke tests with errors that were hard for me to interpret. I have one idea though, make the language server accept options interface that can have for example definitionsFileExists function or something like that. What do you think? Do you have already an idea how those initialization tests should be implemented?

By the way we can also get the language server from nuget in mason-registry. Perhaps that could be to consider as well?

I’ll take a look at #33 after this PR gets merged!

[chrissimon-au]

Hi @erikjuhani - yes, unfortunately this is probably a tough one for a newcomer to F#, and also a newcomer to the LSP and in particular the OmniSharp LSP Client that we are using in the tests! Not to mention, there are some F# abstractions I've added around the TestClient...

In principal, this new test should follow the pattern of the existing tests:

1. Construct a TestClient with the appropriate configuration for the scenario
2. Ask the TestClient to initialise itself (including starting the language server) and then to wait for a log message from the language server to either confirm that the initialisation is complete, or that an expected error message has been logged.
3. Assert on the contents of the reply (which contains the found log message, if found, or None if it times out waiting)

We cannot inject anything into the language server, because the language server is invoked by the language client (TestClient) and then interacted with via the LSP messages. So the server must be able to configure itself only using information it has access to via the LSP messages.

The following test should test the scenario where there is a workspace folder defined but no contextive path defined in the settings (we configure a dummySection config section to force the language client to advertise that it does support providing config settings, but in this case doesn't provide a context.path setting):

          testAsync "Server loads contextive file from default location when no configuration supplied" {
              let config =
                  [ Workspace.optionsBuilder ""
                    ConfigurationSection.optionsBuilder "dummySection" (fun () -> Map []) ]

              let defaultPath = ".contextive/definitions.yml"

              let! (client, reply) = TestClient(config) |> initWithReply

              test <@ (defaultArg reply "").Contains(defaultPath) @>
          }

However, this test doesn't pass with your current implementation - because of line 46:

   if Directory.Exists contextiveFolder

The tests are executing in the build folder, so the workspace at the time of running the tests is a folder that does not contain the .contextive directory.

We could construct a test fixture that simulates that, but for this purpose I'm not sure it's necessary. The existing logic just retrieves the setting value from config, it does not assert on the existence of the file - this is done later when actually loading the file. For consistency, I think we can think of your new method as just constructing the appropriate 'default' setting value - where we expect it should be. And then as it currently works, the file loader which runs later can handle the scenario of the file not existing.

To that end, I'd suggest that the naming of some of the constants and methods you've added should indicate that this part is loading the default value - I'll add some comments on the PR change file to illustrate more concretely.

[chrissimon-au]

As noted on the PR comment, a few suggestions to clear up the purpose of the new methods and to make it easier to test.

[chrissimon-au]

defaultContextiveFolderName ?

[chrissimon-au]

defaultContextiveDefinitionsFileName ?

[chrissimon-au]

getDefaultContextiveDefinitionsPath ?

[erikjuhani]

This is now simply:

[<Literal>]
let defaultContextiveDefinitionsPath = ".contextive/definitions.yml"

[chrissimon-au]

Can remove this check - constructing the default setting value doesn't need to check it's validity. Other parts of the codebase already have to handle the scenario that a configured location doesn't exist.

[chrissimon-au]

In fact, I'm not sure we need to combine with the workspaceFolder at all.

The method below, (getConfig) usually returns a relative path from the setting value supplied by VsCode. So the new getDefaultContextiveDefinitionsPath can just return the relative path as well. Combining with the workspace folder can then happen later in the same way for both cases.

[erikjuhani]

By the way sorry for rebasing this already, the comments are no longer pointing to the right place and in hindsight I should have used fixup commits instead and rebase as the last operation.

I changed this to just be a literal value let defaultContextiveDefinitionsPath = ".contextive/definitions.yml"

[chrissimon-au]

To make things easier, I've also just pushed a new test on main (and renamed initWithReply to initAndWaitForReply to make it clearer what's happening). The new test is kind of testing your scenario, but it asserts that it fails with the expected error message, as per current behaviour.

You can rebase from main and then modify that test to look more like the example test I've included above if you like?

[erikjuhani]

Hi @erikjuhani - yes, unfortunately this is probably a tough one for a newcomer to F#, and also a newcomer to the LSP and in particular the OmniSharp LSP Client that we are using in the tests! Not to mention, there are some F# abstractions I've added around the TestClient...

In principal, this new test should follow the pattern of the existing tests:

1. Construct a TestClient with the appropriate configuration for the scenario

2. Ask the TestClient to initialise itself (including starting the language server) and then to wait for a log message from the language server to either confirm that the initialisation is complete, or that an expected error message has been logged.

3. Assert on the contents of the `reply` (which contains the found log message, if found, or None if it times out waiting)

We cannot inject anything into the language server, because the language server is invoked by the language client (TestClient) and then interacted with via the LSP messages. So the server must be able to configure itself only using information it has access to via the LSP messages.

The following test should test the scenario where there is a workspace folder defined but no contextive path defined in the settings (we configure a dummySection config section to force the language client to advertise that it does support providing config settings, but in this case doesn't provide a context.path setting):

          testAsync "Server loads contextive file from default location when no configuration supplied" {
              let config =
                  [ Workspace.optionsBuilder ""
                    ConfigurationSection.optionsBuilder "dummySection" (fun () -> Map []) ]

              let defaultPath = ".contextive/definitions.yml"

              let! (client, reply) = TestClient(config) |> initWithReply

              test <@ (defaultArg reply "").Contains(defaultPath) @>
          }

Yeah! I'm getting there, I hope! 😅 It's much clearer now after your explanation and example of the test. Thanks for that, it really helped me to move forward with the changes! I think it is correct now. 🤞

However, this test doesn't pass with your current implementation - because of line 46:

   if Directory.Exists contextiveFolder

The tests are executing in the build folder, so the workspace at the time of running the tests is a folder that does not contain the .contextive directory.

We could construct a test fixture that simulates that, but for this purpose I'm not sure it's necessary. The existing logic just retrieves the setting value from config, it does not assert on the existence of the file - this is done later when actually loading the file. For consistency, I think we can think of your new method as just constructing the appropriate 'default' setting value - where we expect it should be. And then as it currently works, the file loader which runs later can handle the scenario of the file not existing.

To that end, I'd suggest that the naming of some of the constants and methods you've added should indicate that this part is loading the default value - I'll add some comments on the PR change file to illustrate more concretely.

Yeah I agree! Now I understand the purpose better and I hope the changes I did reflect exactly what you mean. I removed the two new default constants and merged them as one, to simplify, and also that's what we want anyway.

[erikjuhani]

I did the changes you suggested and rebased the fork with the newest upstream main. Thank you again for the detailed explanation! Like I said it really helped me move forward, something definitely clicked after seeing the example test you provided and reading the steps and how to LSP should be treated.

[chrissimon-au]

Thanks @erikjuhani - this looks great, I've merged the PR now and the github actions are running. I've completed #50 earlier today, so the next release should publish the language server as an independent binary to the github release files, and with this change it should be a language server that 'just works' with neovim 🚀 !