python: Integrate linting tools

[phlax]

Signed-off-by: Ryan Northey ryan@synca.io

Commit Message: python: Integrate linting tools
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

[repokitteh-read-only]

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #15922 was opened by phlax.

see: more, trace.

[phlax]

coverage is here https://storage.googleapis.com/envoy-pr/15922/tooling/index.html

[htuch]

Code looks solid, but there is a lot, so I'm asking for some high-level due diligence before checking this in.

[phlax]

Code ... there is a lot

to be fair, the majority of added code is unit tests. Ive done some things to try and cut this down (ie using "parametrization")

it could be cut further, but only at the cost of a significant increase in complexity - ive tried to balance the complexity <> brevity - the tests inevitably have to be read/changed as the code changes

The other point re the voluminous code. Ive tried to abstract away the common patterns for our needs in terms of python tooling - this makes the actual implementation code much less, and far easier to test. Pretty much all of our current tooling could imho benefit from being rewritten/tested using these classes, which i believe will cut down a significant amount of existing code

[phlax]

we're building a lot of scaffolding here for running checkers; have you done some analysis of what else is out there in the Python ecosystem that might do something similar?

this is certainly something ive thought about quite a lot. There is not easy answers, there are definitely many ways the aims here could have been acheived.

In my thinking/design the requirements of the checker are a super set of the runner - hence their entanglement.

Broadly, the requirements of the runner are:

• Add argparse and parse incoming params (ie as opposed to the far less explicit way of parsing env vars buried in the code)
• Set up logging

I have often lamented that python doesnt have a logging.do_the_right_thing() function - so its either set it up properly or just use print/stdout/stderr - ive opted to use logging - mostly because it gives the end use/r more control over what is actually output.

Regarding argparse - there is another pattern/approach that i could have taken - click

I didnt do this - it would have only provided one aspect of what is required and would enforce another layer/pattern which most likely would not have reduced code. Also it is a functional/procedural approach - which imhe is a lot harder to write tests for. Ultimately its another thing to learn - stdlib is standard lib.

The next candidate in terms of generics would be attrs. Im mindful that some pretty expert opinion argues that no matter what the question attr.s is always the answer - certainly i use this in other projects (eg envoy-playground) and it has some significant benefits. Ultimately i didnt because again it would be unlikely to reduce much code (it neither provides argparse or logging, just a diffferent pattern for writing classes). Ultimately it again adds another layer/pattern to learn and stdlib is...

The checker class adds some additional features/requirements

• run multiple checkers sequentially with the option of only running some
• dont fail on error
• collect up warnings/errors/success during a run
• give decent feedback (ie summary) to the user at the end

i dont know of any generic checker class/es - any search i make returns pytest - which indeed provides these functionalities, but imho is wholly unsuited to the tasks at hand (ie the Checker is not for testing python code - its for checking all manner of code in the repo). The workings of pytest did however provide some inspiration when i was thinking through the required patterns. The other thing that provided some inspiration here is the work that i have done in the past on l10n checkers.

There are also a couple of orthogonal (to both runner and checker) considerations:

• running commands in a subshell
• running bazel commands - ie run/query

wrt to subshell forking - stdlib works just fine here - ive just tried to encapsulate the defaults that are common for the tasks at hand

re bazel - i have searched, pypi and google, and to my surprise there is not a pybazel lib or similar. I was actually quite shocked that i cant just do import bazel; bazel.run('//mytarget'). Ultimately using a subshell kinda works, so its not such a big issue. Also, the point about bazel in this PR is kinda moot as it has not yet been added.

[htuch]

@phlax fair, thanks for the detailed explanation, it sounds like the homework has been done here.

[htuch]

LGTM, modulo comments.

[phlax]

@htuch should be good for final review

[htuch]

LGTM, thanks!

[htuch]

/lgtm deps