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.

Sign up for GitHub

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

Jump to bottom

shorten code reviews with Travis and shellcheck #84

Open
msimerson opened this issue Sep 24, 2016 · 2 comments · May be fixed by #85
Open

shorten code reviews with Travis and shellcheck #84

msimerson opened this issue Sep 24, 2016 · 2 comments · May be fixed by #85

Comments

@msimerson
Copy link

I was reading through a couple PRs and noticed the pointing out of quite a few lint type issues. Basically, stuff a human shouldn't have to be bothered with...

This could be completely automated away, such that when a developer opens a PR, a hook calls Travis which would check out a version of the code and run shellcheck against it.

benefits

  • lint issues are caught automatically, and developers can see what the issues are
  • it makes the continuous integration service (Travis) the strict lint police
  • GitHub will let you prevent PRs that break the tests from being merged ("sorry, can't merge it until the tests pass")

costs

  • you'll need to merge the PR I'll create
  • you'll need to set up Travis access to your repo (so they get notified of PRs, and can post results back to the PRs)
  • there will likely be some lint issues to clean up before tests pass. Basically, a developer will go through the issues raised and decide whether to suppress that particular warning, or alter the code to "fix" it.

Interested?
@aqw
Copy link
Collaborator

aqw commented Sep 24, 2016

@msimerson Thanks for offering to do this. I'm definitely interested. This is something I've been thinking of doing, but never got around to it.

My only concern is whether we ourselves can extend or adjust the linting rules, as our portability requirements impose... interesting requirements upon our shell-style. :-)

Other than that, I'm all for this. I'm traveling this weekend, but will setup a Travis account either Monday or Tuesday when I get back.

Looking forward to this.

---Alex

@msimerson
Copy link
Author

Okay, I'll create a PR for you. You can suppress shellcheck rules that you don't agree with, don't like, or agree with but simply can't avoid (edge cases). I've found that's been plenty of adjustment.

@msimerson msimerson linked a pull request Sep 24, 2016 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

add travis ci testing msimerson/zfsnap
2 participants
@msimerson @aqw