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

Move linting to GitHub Actions with reviewdog. #17143

Merged
merged 1 commit into from Apr 18, 2020

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Apr 15, 2020

PR Summary

This moves flake8 from Travis to GitHub Actions using reviewdog to post it to the GitHub Checks. It runs a bit faster now (basically all time is running flake8 vs ~0 install time) at ~1m vs ~5.5m on Travis.

Linting will appear as a section separate from Travis now in the Checks section of the PR, but I could switch the config to post a review.

Here is the PR I'm using to test out the results.

PR Checklist

  • [N/A] Has Pytest style unit tests
  • Code is Flake 8 compliant
  • [N/A] New features are documented, with examples if plot related
  • [N/A] Documentation is sphinx and numpydoc compliant
  • [N/A] Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • [N/A] Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@QuLogic
Copy link
Member Author

QuLogic commented Apr 15, 2020

So it doesn't post here because the token is for the PR repo and not this repo. I'm not sure if that will fix itself once merged.

Considering also adding cppcheck and eslint, but there's a bug in reviewdog preventing the latter. We could also add some things like misspell or languagetool, but I'm not sure if we want to (hard to say for technical things how useful spell-check is.)

@QuLogic
Copy link
Member Author

QuLogic commented Apr 15, 2020

@tacaswell ran a test PR for me here: QuLogic#5 Unfortunately, PRs don't get writable tokens for Checks API, so it won't post its separate Check with the annotation like in my test.

That's a bit disappointing, but at least it's not hidden inside Travis checks now. There are a couple of other token/review options that I will check, but if none is better, I'll rename some of the stages to make it more obvious what to look at on error.

@tacaswell tacaswell added this to the v3.3.0 milestone Apr 15, 2020
@QuLogic
Copy link
Member Author

QuLogic commented Apr 16, 2020

Ah wait, I'm totally blind. The fallback annotations do show up; they just aren't on the sub-job, but the top-level "Linting" section.

@QuLogic
Copy link
Member Author

QuLogic commented Apr 17, 2020

Updated for #17145, and updated docs due to #17040.

@dopplershift
Copy link
Contributor

If you're looking for spellchecking in code, you can also look at codespell.

@QuLogic
Copy link
Member Author

QuLogic commented Apr 18, 2020

I ran codespell and opened #17184, though there are still several false positives that might impede enabling it on PRs immediately.

@timhoffm
Copy link
Member

Is there still something to do on this PR or can it be merged?

@QuLogic
Copy link
Member Author

QuLogic commented Apr 18, 2020

It can be merged; that's why it's no longer draft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants