sunaku / vim-dasht

:information_desk_person: (Neo)Vim plugin for dasht integration
https://github.com/sunaku/dasht
133 stars 4 forks source link

fix: Do not include file type in docsets when they are set explicitly #18

Closed Holzhaus closed 1 year ago

Holzhaus commented 1 year ago

This change will stop vim-dasht from including the name of the file type in the docsets, because this leads to matches in irrelevant docsets being included in the output.

For example, if the file type is c, all docsets that have a c in its name will be searched.

The user cannot change this behavior from the config, even when setting g:dasht_filetype_docsets:

let g:dasht_filetype_docsets = {}
let g:dasht_filetype_docsets['java'] = ['Java_SE11', 'JUnit5']

If the user has a javascript docset installed, it will still be used for java files despite the configuration.

This changes this behavior to always use only those docsets that have been specified explicitly by the user. If there is no configuration for the current file type, it falls back to the old behavior and searches in all docsets that have the file type name in their name.

sunaku commented 1 year ago

Hello, thanks for this PR! :pray: Unfortunately, this change broke some tests (as follows) and I'm pondering how best to fix them.

$ bundle exec vim-flavor test
-------- Preparing dependencies
Checking versions...
  Use kana/vim-vspec ... 1.9.0
Deploying plugins...
  kana/vim-vspec 1.9.0 ... skipped (already deployed)
Completed.
-------- Testing a Vim plugin
t/dasht_test.vim .. 1/?
not ok 16 - dasht#resolve_docsets resolves filetype to itself and definition in dictionary
# Expected dasht#resolve_docsets('foo') == ['foo'] at line 2
#       Actual value: []
#     Expected value: ["foo"]
not ok 17 - dasht#resolve_docsets resolves filetype for multiple docsets if list is given
# Expected dasht#resolve_docsets(['foo', 'bar']) == ['foo', 'hoge', 'bar', 'piyo'] at line 8
#       Actual value: ["hoge", "piyo"]
#     Expected value: ["foo", "hoge", "bar", "piyo"]
t/dasht_test.vim .. Failed 2/36 subtests

Test Summary Report
-------------------
t/dasht_test.vim (Wstat: 0 Tests: 36 Failed: 2)
  Failed tests:  16-17
Files=1, Tests=36,  0 wallclock secs ( 0.01 usr  0.01 sys +  0.03 cusr  0.01 csys =  0.06 CPU)
Result: FAIL
exit 1
sunaku commented 1 year ago

Alternative: What if we still included the current filetype by default but added stronger ^ and $ regex anchors to dilute their matching power? For example, instead of adding c by default, we would add ^c$ so that only the C docset would be selected.

Holzhaus commented 1 year ago

That would work for my use case, but I think the more flexible approach would be to just change the tests. If the user wants to include those docsets, it's still possible to add that value to the pattern list yourself.

For example, what if I write C code, but usually only want to search library-specific docsets (not the C docset), that would not really be possible.

sunaku commented 1 year ago

Agreed, let's fix the tests and update the documentation (specifically the README) accordingly. Thanks.

sunaku commented 1 year ago

Merged now in commit d3ddf8a347be2881d9e7d71975518f861c1b711b, with very minor additional clean up (per this comment).

Sorry it took me so long to finally merge this... :sweat_smile: and thanks again for your contribution! :bow: