ci: Add GitHub action for style checkers #150

Ana06 · 2020-07-15T09:48:21Z

Run isort 5 and black for every pull request or push.

You can see this working in Ana06/capa

The idea is to add jobs to the same action for rule linter and tests and to show a badge in the README with the status of the workflow (the name of the badge is the same as the workflow - CI). See #151

Closes #75
Closes #76

Ana06 · 2020-07-15T09:49:23Z

.github/workflows/tests.yml

+    - name: Install dependencies
+      run: pip install 'isort==5.*' black
+    - name: Lint with isort
+      run: isort --profile black --length-sort --line-width 120 -c .


I am not sure if we want to modify this options. See #76 (comment) DO NOT MERGE until this is clarified. We should also solve the offenses once this is clarified.

If this does all-in-one, that's cool!

I think it is not. The black profile adapts isort's configuration so that it doesn't conflict with black. But it doesn't run black.

Ah, gotcha!

don't we need to specify the third party modules, like idc, here?

I don't think so, because now it is the default that unrecognise libraries are considered third parties. Note that we were adding argpase as thirdlibrary, but it seems to be in the standard library: https://github.com/timothycrosley/isort/blob/9ee7a97c5a613c7ed536de5635bf26151ba47f55/isort/stdlibs/py35.py#L13

mr-tz

nice, can this be configured to run isort/black and commit the changes back instead of failing the checks?

.github/workflows/tests.yml

mr-tz · 2020-07-15T10:19:12Z

.github/workflows/tests.yml

+    - name: Install dependencies
+      run: pip install 'isort==5.*' black
+    - name: Lint with isort
+      run: isort --profile black --length-sort --line-width 120 -c .


If this does all-in-one, that's cool!

Ana06 · 2020-07-15T14:26:49Z

@mr-tz

nice, can this be configured to run isort/black and commit the changes back instead of failing the checks?

We could, but I find it a bad idea. Taking into account that it will fail in the PR if offenses are introduced, I think it is easy to ensure that new changes don't break the style checker/linter/tests (it can even be enforced by not allowing merges of failing PR and blocking push to master). We already have commits to synchronize submodules, so I would avoid adding more extra commits which are not strictly neccesary. I think having a clean commit history is important:

It helps to know when and why a change/bug was introduced (for example when using git-bisect or GitHub blame/history)
It helps other developers following the repository development/history
It makes reverting changes easier

GitLab has similar arguments:

Git commit messages are the fingerprints that you leave on the code you touch. Any code that you commit today, a year from now when you look at the same change; you would be thankful for a clear, meaningful commit message that you wrote, and it will also make the lives of your fellow developers easier. When commits are isolated based on context, a bug which was introduced by a certain commit becomes quicker to find, and the easier it is to revert the commit which caused the bug in the first place.

Note also that this wouldn't work anyway for rule linters and tests, as they can't be solved automatically.

mr-tz · 2020-07-15T14:38:51Z

very good, thanks for the details here!

williballenthin · 2020-07-15T15:43:31Z

good argument @Ana06 - i'm convinced.

so we'll run black & isort in "check" mode, failing the CI run if they don't succeed, which will block a PR. then its up to the author to fix before merging. makes sense to me.

williballenthin · 2020-07-15T15:44:11Z

given that its up to the author to do the formatting, we should document the commands that we recommend to do isort & block in auto-fix mode.

Ana06 · 2020-07-16T16:55:51Z

@williballenthin

given that its up to the author to do the formatting, we should document the commands that we recommend to do isort & block in auto-fix mode.

we should also update the hooks.

Ana06 · 2020-07-16T17:05:50Z

@williballenthin @mr-tz If you are fine with the changes in f4e6541 we can merge this. Some changes seems related to argparse not being a third party library. The rest seems to be improvements in the new version. I also think we don't need to specify --third party because now it is the default that unrecognized libraries are considered third parties.

I will then open an issue to update the hooks.

williballenthin · 2020-07-16T17:29:27Z

go ahead and merge CI changes as you see fit to get this working

Run `isort --profile black --length-sort --line-width 120 .` Update documentation as well.

Run isort and black for every pull request or push.

Ana06 · 2020-07-16T20:19:44Z

GitHub Actions are working! 🥳 As @williballenthin said I can merge it... there we go! 🚢

Ana06 added enhancement New feature or request dont merge Indicate a PR that is still being worked on CI Continuous Integration configuration labels Jul 15, 2020

Ana06 requested review from williballenthin and mr-tz July 15, 2020 09:48

Ana06 commented Jul 15, 2020

View reviewed changes

Ana06 mentioned this pull request Jul 15, 2020

Add CI status badge to README #151

Merged

mr-tz reviewed Jul 15, 2020

View reviewed changes

Ana06 force-pushed the ana-code-style branch 2 times, most recently from 6c4b1f5 to d484c08 Compare July 15, 2020 13:07

Ana06 force-pushed the ana-code-style branch 2 times, most recently from 7607b41 to 91f577f Compare July 16, 2020 17:02

Ana06 removed the dont merge Indicate a PR that is still being worked on label Jul 16, 2020

Ana06 force-pushed the ana-code-style branch from 91f577f to f4e6541 Compare July 16, 2020 17:07

Ana06 force-pushed the ana-code-style branch from f4e6541 to 7913e97 Compare July 16, 2020 20:00

Ana06 added 2 commits July 16, 2020 22:02

Use isort 5

07764fb

Run `isort --profile black --length-sort --line-width 120 .` Update documentation as well.

ci: Add GitHub action for style checkers

4ab97ec

Run isort and black for every pull request or push.

Ana06 force-pushed the ana-code-style branch from 7913e97 to 4ab97ec Compare July 16, 2020 20:06

Ana06 merged commit ef44e7e into master Jul 16, 2020

Ana06 deleted the ana-code-style branch July 16, 2020 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: Add GitHub action for style checkers #150

ci: Add GitHub action for style checkers #150

Ana06 commented Jul 15, 2020 •

edited

Loading

Ana06 Jul 15, 2020 •

edited

Loading

mr-tz Jul 15, 2020

Ana06 Jul 15, 2020

mr-tz Jul 15, 2020

williballenthin Jul 15, 2020

Ana06 Jul 16, 2020

mr-tz left a comment

mr-tz Jul 15, 2020

Ana06 commented Jul 15, 2020 •

edited

Loading

mr-tz commented Jul 15, 2020

williballenthin commented Jul 15, 2020

williballenthin commented Jul 15, 2020

Ana06 commented Jul 16, 2020

Ana06 commented Jul 16, 2020 •

edited

Loading

williballenthin commented Jul 16, 2020

Ana06 commented Jul 16, 2020

ci: Add GitHub action for style checkers #150

ci: Add GitHub action for style checkers #150

Conversation

Ana06 commented Jul 15, 2020 • edited Loading

Ana06 Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

mr-tz Jul 15, 2020

Choose a reason for hiding this comment

Ana06 Jul 15, 2020

Choose a reason for hiding this comment

mr-tz Jul 15, 2020

Choose a reason for hiding this comment

williballenthin Jul 15, 2020

Choose a reason for hiding this comment

Ana06 Jul 16, 2020

Choose a reason for hiding this comment

mr-tz left a comment

Choose a reason for hiding this comment

mr-tz Jul 15, 2020

Choose a reason for hiding this comment

Ana06 commented Jul 15, 2020 • edited Loading

mr-tz commented Jul 15, 2020

williballenthin commented Jul 15, 2020

williballenthin commented Jul 15, 2020

Ana06 commented Jul 16, 2020

Ana06 commented Jul 16, 2020 • edited Loading

williballenthin commented Jul 16, 2020

Ana06 commented Jul 16, 2020

Ana06 commented Jul 15, 2020 •

edited

Loading

Ana06 Jul 15, 2020 •

edited

Loading

Ana06 commented Jul 15, 2020 •

edited

Loading

Ana06 commented Jul 16, 2020 •

edited

Loading