[WIP] Use pre-commit to run linters #1912

ssbarnea · 2019-04-09T20:06:52Z

Runs linters via pre-commit, which provides few extra benefits:

linters are pinned (also avoiding site-packages related bugs)
easy bumping via pre-commit autoupdate
speed-up on subsequent executions
avoid the need to run tox -r when updating linter versions

Side effects:

Fixes inconsequent yaml identation
Fixes missordering of dependecies
Fixes extra newlines on few files

Signed-off-by: Sorin Sbarnea [email protected]

Please include details of what it is, how to use it, how it's been tested

PR Type

Feature Pull Request

Runs linters via pre-commit, which provides few extra benefits: * linters are pinned (also avoiding site-packages related bugs) * easy bumping via `pre-commit autoupdate` * speed-up on subsequent executions * avoid the need to run tox -r when updating linter versions Side effects: * Fixes inconsequent yaml identation * Fixes missordering of dependecies * Fixes extra newlines on few files Signed-off-by: Sorin Sbarnea <[email protected]>

ssbarnea · 2019-04-09T20:08:05Z

.yamllint

@@ -10,4 +10,7 @@ rules:
  brackets:
    max-spaces-inside: 1
    level: error
+  document-start: disable


almost nobody is using the --- prefix and adding it would touch a huge amount of files. we can do it, but in another change.

ssbarnea · 2019-04-09T20:08:55Z

.yamllint

  line-length: disable
+  key-duplicates: enable
+  truthy: disable


truthy is debatable too and does not play well with "on" use on travis config, better off than adding exclude for travis file.

MarkusTeufelberger · 2019-04-09T20:29:58Z

Why run the linters through yet another external tool (that requires extra config checked in as well) instead of adding a few jobs that run each linter in parallel? It's not like travis-ci is going to install pre-commit hooks...

decentral1se

Personally, I would also like to avoid another configuration file. I am fine with tox and I am not sure the benefits you mentioned can't be done without using this extra tool. Perhaps introducing another tool to the developer workflow is worth asking on the IRC channel first.

webknjaz · 2019-04-09T23:19:08Z

.readthedocs.yml

@@ -3,5 +3,5 @@ build:
 python:
  version: 3.6
  extra_requirements:
-  - docs
+    - docs


I'd like to avoid indenting this.

...why? It is the default in yamllint and more readable too (since the list is a child item it should be indented). Also the yaml files used by molecule are styled like this.

I'd argue that too much indentation creates more distraction and more ugliness. Also, since the files changed are mostly maintained by me I'll be insisting.
You'll see when you end up with 10 levels of indentation someday.

It's fine to lint that it's parsable, though.

So we need a "files that webknjaz maintains" exclude for yamllint? This is the second person who wants to indent this vs. your "I maintain (own!?) this file" mentality. Don't stop imparting your wisdom on this project, please, but your subjectivities around indentation may be having less interesting effects. I only speak for myself ...

Well, you were caring about contributor experience. So from this PoV it's not logical to make things unreadable for the contributor who looks there often vs. the possibility of somebody else looking at it once or twice in the next decade. I wouldn't care if I were to fully defer the related responsibilities to someone else.

exclude for yamllint

a separate config would do

P.S. In the internal Ansible Core team Slack it looks like the majority of folks prefer non-indented sequences (as per recent discussions). We also mostly disagree with a lot of what ansible-lint pushes by default. I think this may lead to PoV diversity.

I see a number of comments telling me about experience now. Please do not use this as a bat to beat me with. My personal experience on this project is more or less fine except for trying to guess at what others might find easier and then hearing the fall out from it ... but I am going to stop doing that now.

The travis file is also heavily (and unnecessarily in my opinion) parametrized with tons of single line anchors etc.

I wouldn't see the indentation of the file as the main source of its distractiveness and ugliness.

P.S. In the internal Ansible Core team Slack it looks like the majority of folks prefer non-indented sequences (as per recent discussions). We also mostly disagree with a lot of what ansible-lint pushes by default. I think this may lead to PoV diversity.

Then the internal slack group is free to comment here as well and give their reasons. I already wrote mine above (having the -on the same indentation level while being a child item makes it less obvious that this is a child element). Also this is about yamllint, not ansible-lint.

As a data point: https://yaml.org/spec/1.2/2009-07-21/spec.html#id2559116 indents child lists.

webknjaz · 2019-04-09T23:19:58Z

.yamllint

@@ -10,4 +10,7 @@ rules:
  brackets:
    max-spaces-inside: 1
    level: error
+  document-start: disable


I think this could go to a separate discussion in a separate PR.

webknjaz · 2019-04-09T23:20:58Z

.flake8

@@ -1,4 +1,5 @@
 [flake8]
+# keep exclude in-sync with ones in .pre-commitconfig.yaml or better remove them


Suggested change

# keep exclude in-sync with ones in .pre-commitconfig.yaml or better remove them

# keep exclude in-sync with ones in .pre-commit-config.yaml or better remove them

webknjaz · 2019-04-09T23:21:18Z

.flake8

@@ -1,4 +1,5 @@
 [flake8]
+# keep exclude in-sync with ones in .pre-commitconfig.yaml or better remove them


Better keep them in just one place.

webknjaz · 2019-04-09T23:21:32Z

.github/ISSUE_TEMPLATE/proposal.md

@@ -1,7 +1,7 @@
 ---
 name: Proposal
 about: Suggest an large change for the Molecule project
-labels: proposal 
+labels: proposal


Separate this plz

webknjaz · 2019-04-09T23:22:32Z

.travis.yml

-  - $HOME/.rvm
-  - $HOME/virtualenv/python$(python -c 'import platform; print(platform.python_version())')
-  - $HOME/Library/Caches/Homebrew
+    - $HOME/.cache/pre-commit


I don't like this indentation.

webknjaz

pre-commit sounds good but I disagree on indentation changes.

webknjaz · 2019-04-09T23:27:10Z

@decentral1se pre-commit is a de-facto standard among popular pythonic repos. It's a good tool and I'm using it for years.

Among the advantages I like:

consolidation of various linters in one place;
ability to set it up as a pre-commit git hook which would invoke it whenever you make a commit or push locally;
ability to run exactly the same checks as in CI locally. it might sound as it's conflicting with tox but in this case, "tox" is just a glue for and a unified interface for invocation of different tasks.

MarkusTeufelberger · 2019-04-09T23:34:36Z

in this case, "tox" is just a glue for and a unified interface for invocation of different tasks

pre-commit is different from this description how exactly? Also in Ansible it seems to be only used in ansible-lint - and there also not really to run/configure any hooks. They use tox instead to run tests which is a de-facto standard among popular pythonic repos.

webknjaz · 2019-04-10T08:42:37Z

@MarkusTeufelberger tox is a runner for different targets (tests, builds, linters). pre-commit is a thing which nicely consolidates linters and tox will run it.
@ssbarnea brings in another ~15 checks/linters: would you prefer to configure each of them separately in a ton of different places? And then pollute tox envs with that?

use tox instead to run tests which is a de-facto standard among popular pythonic repos.

By the way, since you brought it up, one of the tox maintainers actually wrote pre-commit tool and `tox project itself uses it.

Look here:

Are any of these projects popular and pythonic enough for you?

ssbarnea · 2019-04-10T08:52:30Z

@MarkusTeufelberger You need to play a little bit with pre-commit to understand why is so good at what it does. I could say that the only issue I had with it was its name. Due to its name most people assume is just a git-commit-hook, which historically proved to be quite controversial.

In fact the hook part is totally optional but the real value is the ability to manage all linters in a single place, with pinning, with one command bumping (that is not affected by tox virtenv bugs), with super caching (it will reuse a specific linter version across multiple projects, lowering the footprint on developer machines considerably). Also it has the ability to lint only modified files which is of HUGE benefit for slow tools.

Now I can only hope that @webknjaz can be convinced to accept consistent identation on all yaml files in the repo, which has the side effect of fixing the travis config file.

ssbarnea · 2019-04-10T10:03:42Z

I will rework this once I get two preparatory changes merged: #1918 and #1919

MarkusTeufelberger · 2019-04-10T10:03:48Z

@webknjaz - As I wrote: the only Ansible project that has a config for this tool (and an extremely minimal at that) is ansible-lint, which is actually linted and tested using tox.

My main concern is that it gets harder to know which linter or check actually fails if some tool runs a lot of them at once in a single step.

webknjaz · 2019-04-10T18:07:59Z

Now I can only hope that @webknjaz can be convinced to accept consistent identation on all yaml files in the repo, which has the side effect of ~~fixing~~ the travis config file.

s/fixing/breaking/

My main concern is that it gets harder to know which linter or check actually fails if some tool runs a lot of them at once in a single step.

@MarkusTeufelberger: as @ssbarnea wrote, if you play with it yourself you'll understand. It actually very clearly outputs the info about what failed. So you may cross this concern out from your list :)

Example successful output: https://travis-ci.org/cherrypy/cherrypy/jobs/512712187#L483-L501
Example failure: https://travis-ci.org/cherrypy/cheroot/jobs/518115081#L239-L241

the only Ansible project that has a config for this tool

Just because I didn't get to it :)

MarkusTeufelberger · 2019-04-10T19:13:40Z

It actually very clearly outputs the info about what failed.

Yeah, but you need to view the output for that. I am talking about the different checks that are displayed in GitHub above (the "Some checks were not successful - 2 action required, 1 in progress, and 1 successful checks" part below). There it would be just a generic failed task just like now, ideally there would be a list of different linters so it is easy to see if yamllint or ansible-lint or flake8 or whatever were the one complaining. These jobs also would be spawned by Travis in parallel.

Having to check the build logs for which linter actually is the one that failed doesn't sound "very clearly" to me at all.

Edit: https://travis-ci.org/cherrypy/cheroot/jobs/518115081 is also a good example for "something failed in some tool, now go hunt through the log for the actual error location". You linked to line ~240, the log has ~1100 lines. The actual error location requires to read upwards for about 80% (!) of the build log.

webknjaz · 2019-04-10T19:18:34Z

@MarkusTeufelberger I agree with that in general and you could separate things if you wanted. But the point here is that if you bring these things into tox directly you'll pollute it.
And also take into account that most of the integrations (and Travis in particular) don't report separate checks to GitHub as they should.
I think I may want to write a plugin for tox to improve that (I've started https://tutorial.octomachinery.dev because of this after all). But now this concern isn't relevant as we cannot address it. Not yet.

webknjaz · 2019-04-10T19:20:08Z

tox.ini

@@ -37,8 +37,7 @@ commands_pre =
 commands =
    unit: pytest test/unit/ --cov={toxinidir}/molecule/ --no-cov-on-fail {posargs}
    functional: pytest test/functional/ {posargs}
-    lint: flake8
-    lint: yamllint -s test/ molecule/
+    lint: python -m pre_commit run -a


Put this under lint env.

(and please use verbose --all-files)

MarkusTeufelberger · 2019-04-11T10:51:20Z

And also take into account that most of the integrations (and Travis in particular) don't report separate checks to GitHub as they should.

Once they do (or a different CI tool is used, once Travis is killed properly by their new owners), are you commiting to remove this pre-commit stuff from the repository again?

Edit:
Oh, and I see that you have pushed a few unreviewed things into this repo that make you "owner" of several files: #1928 - is this in reaction to this PR, or why has this been done?

webknjaz · 2019-04-11T11:16:33Z

@MarkusTeufelberger nope. Do you have any other standard and easy to set up tool to hook up with Git?

webknjaz · 2019-04-11T11:22:49Z

why has this been done

It's just a config for GitHub so that it'd auto-request a review from me. I hope you're not confusing the term "owner". In software projects, it often is synonymous with "responsible party". The reason is that I want to get notified about changes in certain files more granularly.

MarkusTeufelberger · 2019-04-11T11:23:49Z

GitLab-CI for example. If you want a place to run several checks or types of checks at once, you can use tox, it is already set up in this repository... This feels like talking in circles though. Integrate pre-commit if you like, still it would be a better experience if the individual linters get pulled apart in Travis-CI into separate tasks that can fail separately (and ensuring that all are run! Does pre-commit stop on the first failing linter?) instead of having one single large lint job/stage.

webknjaz · 2019-04-11T11:49:03Z

Does pre-commit stop on the first failing linter?

No, it runs all of them.

a better experience if the individual linters get pulled apart in Travis-CI into separate tasks that can fail separately

I know, but the second part of the coin is polluting Travis' per-repo and per-org queues which in turn make the experience worse :(
If only we had unlimited resources, I'd gladly vote for one fail per job, though.

I think I may achieve this with GitHub Actions, btw. Let me think about it separately.

I think the main point here is the ability to have pre-commit hooks wired locally with a battle-tested tool.

webknjaz · 2019-04-25T14:52:56Z

Superseded by #1996

ssbarnea commented Apr 9, 2019

View reviewed changes

decentral1se suggested changes Apr 9, 2019

View reviewed changes

webknjaz reviewed Apr 9, 2019

View reviewed changes

webknjaz suggested changes Apr 9, 2019

View reviewed changes

ssbarnea changed the title ~~Use pre-commit to run linters~~ [WIP] Use pre-commit to run linters Apr 10, 2019

ssbarnea added the test Improvement to quality assurance: CI/CD, testing, building label Apr 10, 2019

ssbarnea self-assigned this Apr 10, 2019

webknjaz reviewed Apr 10, 2019

View reviewed changes

webknjaz closed this Apr 25, 2019

webknjaz deleted the fix/pre-commit branch April 25, 2019 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Use pre-commit to run linters #1912

[WIP] Use pre-commit to run linters #1912

ssbarnea commented Apr 9, 2019

ssbarnea Apr 9, 2019

ssbarnea Apr 9, 2019

MarkusTeufelberger commented Apr 9, 2019

decentral1se left a comment

webknjaz Apr 9, 2019

MarkusTeufelberger Apr 9, 2019

webknjaz Apr 10, 2019

decentral1se Apr 11, 2019

webknjaz Apr 11, 2019

decentral1se Apr 11, 2019 •

edited

Loading

MarkusTeufelberger Apr 11, 2019

webknjaz Apr 9, 2019

webknjaz Apr 9, 2019

webknjaz Apr 9, 2019

webknjaz Apr 9, 2019

webknjaz Apr 9, 2019

webknjaz left a comment

webknjaz commented Apr 9, 2019

MarkusTeufelberger commented Apr 9, 2019

webknjaz commented Apr 10, 2019

ssbarnea commented Apr 10, 2019

ssbarnea commented Apr 10, 2019

MarkusTeufelberger commented Apr 10, 2019

webknjaz commented Apr 10, 2019

MarkusTeufelberger commented Apr 10, 2019 •

edited

Loading

webknjaz commented Apr 10, 2019

webknjaz Apr 10, 2019

webknjaz Apr 10, 2019

MarkusTeufelberger commented Apr 11, 2019 •

edited

Loading

webknjaz commented Apr 11, 2019

webknjaz commented Apr 11, 2019

MarkusTeufelberger commented Apr 11, 2019

webknjaz commented Apr 11, 2019

webknjaz commented Apr 25, 2019

		@@ -1,4 +1,5 @@
		[flake8]
		# keep exclude in-sync with ones in .pre-commitconfig.yaml or better remove them

[WIP] Use pre-commit to run linters #1912

[WIP] Use pre-commit to run linters #1912

Conversation

ssbarnea commented Apr 9, 2019

PR Type

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkusTeufelberger commented Apr 9, 2019

decentral1se left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

decentral1se Apr 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz left a comment

Choose a reason for hiding this comment

webknjaz commented Apr 9, 2019

MarkusTeufelberger commented Apr 9, 2019

webknjaz commented Apr 10, 2019

ssbarnea commented Apr 10, 2019

ssbarnea commented Apr 10, 2019

MarkusTeufelberger commented Apr 10, 2019

webknjaz commented Apr 10, 2019

MarkusTeufelberger commented Apr 10, 2019 • edited Loading

webknjaz commented Apr 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkusTeufelberger commented Apr 11, 2019 • edited Loading

webknjaz commented Apr 11, 2019

webknjaz commented Apr 11, 2019

MarkusTeufelberger commented Apr 11, 2019

webknjaz commented Apr 11, 2019

webknjaz commented Apr 25, 2019

decentral1se Apr 11, 2019 •

edited

Loading

MarkusTeufelberger commented Apr 10, 2019 •

edited

Loading

MarkusTeufelberger commented Apr 11, 2019 •

edited

Loading