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

Lint vimscript, fix errors and warnings, add CI job to review PRs #1071

Merged
merged 10 commits into from
Jan 2, 2020

Conversation

alerque
Copy link
Member

@alerque alerque commented Dec 31, 2019

Linting is not as good as full on integration testing, but this is a start. The vint linter for VimL is actually pretty good. It's noisy the first time you run it because it's picking about idiosyncrasies of VIM's language and wants all code to avoid even potential breakage, but it is all good practice and once everything is fixed it's pretty easy to use. Once installed it can be run locally using vint . from the project root.

The CI job included here will run the linter in Github Actions and a bot will leave comments on PRs on the line of code where any lint issues are found.

There are a couple flat out bugs that it caught, but most of the changes here were just fixing warnings. I highly recommend this PR not be squashed! This touches a TON of code, but each commit is split out by the kind of lint issue being fixed and it will be much easier to understand the git blame output and potentially git bisect if a problem was introduced if they are not lumped together.

Please do make sure the plugin still runs as expected. I don't usually use it myself so I'm not a good one to put it through even quick paces.

If you poor through the diff it's probably a good idea to do so a commit at a time so it's not just a mess of changes ;-)

After it is merged I would recommend setting master up as a protected branch and requiring the CI job to pass on all PRs before merger. I can set that up in the Github settings is you approve (but I wont' mess with it unless you give the word).

Copy link
Member

@PhilRunninger PhilRunninger left a comment

Choose a reason for hiding this comment

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

Looks good to me. Checks out on Mac and Windows.

@PhilRunninger PhilRunninger merged commit 97fb849 into master Jan 2, 2020
@PhilRunninger PhilRunninger deleted the vint branch January 2, 2020 07:12
@PhilRunninger
Copy link
Member

@alerque , I like the linting that you've added. That will be very handy going forward.

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