search

artempyanykh / marksman

Write Markdown with code assist and intelligence in the comfort of your favourite editor.
MIT License
1.98k stars 35 forks source link

Support configurable completion candidates #315

Closed majjoha closed 2 months ago

majjoha commented 4 months ago

Firstly, thank you for an absolutely amazing piece of software!

In https://github.com/artempyanykh/marksman/pull/233, @narutohahahaha asked if it would be possible to add support for allowing users to configure the number of completion candidates in the .marksman.toml configuration file. Is this something you'd consider?

If somebody wanted to try and implement it, where would they have to start? Would it be a matter of updating https://github.com/artempyanykh/marksman/blob/main/Marksman/Config.fs with an additional field for completion candidates, and then update this line to read the value from the configuration file (and if not set, default to 50) instead, or is there more to it than that?

artempyanykh commented 3 months ago

Hey @majjoha, thanks for the kind words!

Re: implementing the feature, I'd also add 'write a couple tests' 😉 but otherwise that'd be pretty much it.

majjoha commented 3 months ago

Sounds good!

I've given it a go in https://github.com/artempyanykh/marksman/pull/316, but as I have close to no prior experience with F# or LSPs, I could use a few pointers on how to finish the feature. Currently, the implementation seems to not read the candidates field in the configuration file correctly, as it still uses the default value which is now set to 50. Similarly, in the tests, Marksman.ConfigTests.testDefault and Marksman.ConfigTests.testParse_7 in ConfigTests.fs, the expected value is not matched, as the result is null for both tests.

Is there anything obvious that I am doing wrong? :-)

artempyanykh commented 3 months ago

Thanks for having a stab @majjoha ! I commented on the PR