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

cmd/govim: add tests to verify turning off unimported completions works (#687) #796

Merged
merged 1 commit into from
Feb 24, 2020

Conversation

tjcain
Copy link
Contributor

@tjcain tjcain commented Feb 24, 2020

Following d80f0e2 unimported completions are turned on by default. This
commit adds tests to ensure setting CompleteUnimported to false disables
completion for unimported packages.

Copy link
Member

@myitcv myitcv left a comment

Choose a reason for hiding this comment

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

@tjcain - thanks very much. Just a couple of questions/comments.

@tjcain tjcain force-pushed the tests_unimported_completion_off branch from fc6e1a8 to 004b382 Compare February 24, 2020 11:21
Copy link
Member

@myitcv myitcv left a comment

Choose a reason for hiding this comment

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

LGTM. Will merge once build goes green.

Thank you, @tjcain!

@myitcv
Copy link
Member

myitcv commented Feb 24, 2020

@tjcain - sorry, one thing I've just spotted (reviewing the commit message is not, sadly, part of GitHub's flow here, unlike Gerrit).

The (#xxx) suffix is added to commits automatically by GitHub because we are using "squash and merge". But despite using that feature per the wiki we rely on the commit message being "good", otherwise if left unsquashed the person doing the merge has to do the hard work of coming up with a good message!

Your message is great, with the exception of some really small nits:

  • the superfluous suffix on the first line - this will simply be confusing when the PR is merged because it refers to the issue you are fixing. It will become the PR number itself so the linear history of govim can be quickly and easily traced back to the PRs that created commits
  • the first line is a bit verbose
  • the message is missing the "Fixes" directive

Therefore, how about something like?

cmd/govim: verify turning off unimported completions works

Following d80f0e2 unimported completions are turned on by default. This
commit adds tests to ensure setting CompleteUnimported to false disables
completion for unimported packages.

Fixes #687

That way #687 will be closed automatically when we merged.

Thanks

@tjcain
Copy link
Contributor Author

tjcain commented Feb 24, 2020

@tjcain - sorry, one thing I've just spotted (reviewing the commit message is not, sadly, part of GitHub's flow here, unlike Gerrit).

I will get that fixed up now, thank you for the great feedback.

Following d80f0e2 unimported completions are turned on by default. This
commit adds tests to ensure setting CompleteUnimported to false disables
completion for unimported packages.

Fixes govim#687
@tjcain tjcain force-pushed the tests_unimported_completion_off branch from 004b382 to 6caa688 Compare February 24, 2020 11:42
@myitcv
Copy link
Member

myitcv commented Feb 24, 2020

Will ignore the macOS flake for now

@myitcv myitcv merged commit 9f14650 into govim:master Feb 24, 2020
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.

2 participants