Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

separate gometalinter error list use cases #1652

Merged
merged 3 commits into from
Jan 28, 2018

Conversation

bhcleek
Copy link
Collaborator

@bhcleek bhcleek commented Jan 19, 2018

gometalinter is used in two distinct cases. When run using
:GoMetaLinter, all of its output is used. When run automatically on save
(e.g. g:go_metalinter_autosave=1), only its output that applies to the
current buffer is used. In that case, it makes sense to show its
messages in the location list without forcing the user to also use the
location list when the user executes :GoMetaLinter.

Therefore, add a new key to g:go_list_type_commands,
GoMetaLinterAutoSave, with a default value of locationlist and modify
go#lint#Gometa to distinguish between the two cases.

Fixes #1644

@codecov-io
Copy link

codecov-io commented Jan 19, 2018

Codecov Report

Merging #1652 into master will increase coverage by 0.27%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1652      +/-   ##
==========================================
+ Coverage   21.73%   22.01%   +0.27%     
==========================================
  Files          53       53              
  Lines        4173     4179       +6     
==========================================
+ Hits          907      920      +13     
+ Misses       3266     3259       -7
Flag Coverage Δ
#nvim 17.1% <55.55%> (+0.21%) ⬆️
#vim74 19.59% <55.55%> (+0.21%) ⬆️
#vim80 20.79% <55.55%> (+0.18%) ⬆️
Impacted Files Coverage Δ
autoload/go/list.vim 74.66% <ø> (+8%) ⬆️
autoload/go/lint.vim 49.18% <100%> (+2.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ffb2f43...5743ce1. Read the comment docs.

gometalinter is used in two distinct cases. When run using
:GoMetaLinter, all of its output is used. When run automatically on save
(e.g. g:go_metalinter_autosave=1), only its output that applies to the
current buffer is used. In that case, it makes sense to show its
messages in the location list without forcing the user to also use the
location list when the user executes :GoMetaLinter.

Therefore, add a new key to g:go_list_type_commands,
GoMetaLinterAutoSave, with a default value of locationlist and modify
go#lint#Gometa to distinguish between the two cases.

Fixes fatih#1644
@bhcleek bhcleek force-pushed the metalinter-autosave-list branch from 0e6f51b to 4b146cb Compare January 20, 2018 02:58
"quickfix" and "locationlist".
"GoMetaLinter", "GoMetaLinterAutoSave", "GoModifyTags" (used for both
:GoAddTags and :GoRemoveTags), "GoRename", "GoRun", and "GoTest". Supported
values for each command are "quickfix" and "locationlist".
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also update the doc for g:go_metalinter_autosave setting and :GoMetaLinterAutoSaveToggle command to add a notice that locationlist is used instead of quickfix. These two:

                                                  *'g:go_metalinter_autosave'*

Use this option to auto |:GoMetaLinter| on save. Only linter messages for
the active buffer will be shown. By default it's disabled >

  let g:go_metalinter_autosave = 0
<

and

                                                 *:GoMetaLinterAutoSaveToggle*
:GoMetaLinterAutoSaveToggle

    Toggles |'g:go_metalinter_autosave'|.

It's worth adding this because for example I use quickfix for everything and have custom mappings to select next/previous items from the list. So people should be aware of the side effects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

let start = reltime()
while len(actual) == 0 && reltimefloat(reltime(start)) < 10
sleep 100m
let actual = getqflist()
let actual = getloclist(l:winnr)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These commands should be not removed, instead it should be based on go#list#Type("GoMetaLinterAutoSave") and act accordingly, whether it's a location list or quickfix list.

Copy link
Collaborator Author

@bhcleek bhcleek Jan 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is only about the autosave case, and it only tests with the default list type GoMetaLinterAutoSave. Why would it check the list type before clearing the list?

let winnr = winnr()

" clear the location lists
call setloclist(l:winnr, [], 'r')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. We should have a switch and call setqflist() or setloclist() based go#list#Type("GoMetaLinterAutoSave")

Copy link
Collaborator Author

@bhcleek bhcleek Jan 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above

Copy link
Collaborator Author

@bhcleek bhcleek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL

@bhcleek bhcleek merged commit 5743ce1 into fatih:master Jan 28, 2018
@bhcleek bhcleek deleted the metalinter-autosave-list branch January 28, 2018 15:48
bhcleek added a commit that referenced this pull request Jan 29, 2018
Update CHANGELOG.md for #1652, #1653, #1656, and #1664.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants