Time to add pull request linting?

[JuliaKukulies]

A while ago we have discussed the use of pylint, e.g. require a certain score as an additional check in the workflow on pull requests. I think it could be a good time to introduce the use of linting because this would facilitate the review of comprehensive pull requests - a lot of comments tend to be style comments, redundant imports, etc. that would be automatically detected.

The drawback is that this would introduce an additional barrier for code contributors. So instead of making it an official requirement, we could also just recommend the use of linting in the developer guide and try to use it internally first. Any thoughts on this?

[w-k-jones]

I think it would be good to add (and it's interesting to see what it brings up), however I agree that it may add obstacles to contributing to tobac

Would it be possible to add an action that runs pylint and posts the results to the PR and highlight if the score has reduced? That way we could highlight linting issues without it being an immediate barrier to contributing

For reference, the current RC_v1.5.0 branch is rated 5.99/10, the "core" elements (feature detection, segmentation and tracking) get 7.05/10

[freemansw1]

I have a dumb question- what does pylint buy us over Black? I know that pylint is a linter, but will we end up with headaches between PEP-8 style and Black style, or are those compatible?

[w-k-jones]

pylint performs static code analysis, so it can look into more than just formatting. It should be fine with black afaik.

Here's an example below of running pylint on tracking.py. Although some items can be ignored, it does raise some useful points regarding unused imports etc.

pylint tobac/tracking.py 
************* Module tobac.tracking
tobac/tracking.py:205:0: C0301: Line too long (105/100) (line-too-long)
tobac/tracking.py:216:0: C0301: Line too long (183/100) (line-too-long)
tobac/tracking.py:224:0: C0301: Line too long (183/100) (line-too-long)
tobac/tracking.py:228:0: C0301: Line too long (104/100) (line-too-long)
tobac/tracking.py:254:0: C0301: Line too long (116/100) (line-too-long)
tobac/tracking.py:260:0: C0301: Line too long (116/100) (line-too-long)
tobac/tracking.py:272:0: C0301: Line too long (104/100) (line-too-long)
tobac/tracking.py:391:0: C0301: Line too long (130/100) (line-too-long)
tobac/tracking.py:39:4: C0103: Argument name "dt" doesn't conform to snake_case naming style (invalid-name)
tobac/tracking.py:41:4: C0103: Argument name "dz" doesn't conform to snake_case naming style (invalid-name)
tobac/tracking.py:36:0: R0913: Too many arguments (20/5) (too-many-arguments)
tobac/tracking.py:36:0: R0914: Too many local variables (43/15) (too-many-locals)
tobac/tracking.py:233:8: C0103: Variable name "is_3D" doesn't conform to snake_case naming style (invalid-name)
tobac/tracking.py:241:0: W1404: Implicit string concatenation found in call (implicit-str-concat)
tobac/tracking.py:248:8: C0103: Variable name "is_3D" doesn't conform to snake_case naming style (invalid-name)
tobac/tracking.py:266:4: W1201: Use lazy % formatting in logging functions (logging-not-lazy)
tobac/tracking.py:394:31: R1735: Consider using '{}' instead of a call to 'dict'. (use-dict-literal)
tobac/tracking.py:408:21: R1734: Consider using [] instead of list() (use-list-literal)
tobac/tracking.py:415:12: W1201: Use lazy % formatting in logging functions (logging-not-lazy)
tobac/tracking.py:36:0: R0912: Too many branches (35/12) (too-many-branches)
tobac/tracking.py:36:0: R0915: Too many statements (97/50) (too-many-statements)
tobac/tracking.py:38:4: W0613: Unused argument 'field_in' (unused-argument)
tobac/tracking.py:49:4: W0613: Unused argument 'order' (unused-argument)
tobac/tracking.py:455:4: C0103: Argument name "t" doesn't conform to snake_case naming style (invalid-name)
tobac/tracking.py:454:0: R0913: Too many arguments (6/5) (too-many-arguments)
tobac/tracking.py:454:0: R0914: Too many local variables (23/15) (too-many-locals)
tobac/tracking.py:485:4: C0415: Import outside toplevel (scipy.interpolate.InterpolatedUnivariateSpline) (import-outside-toplevel)
tobac/tracking.py:535:18: C0103: Argument name "t" doesn't conform to snake_case naming style (invalid-name)
tobac/tracking.py:550:4: W0612: Unused variable 't_grouped' (unused-variable)
tobac/tracking.py:26:0: C0411: standard import "import warnings" should be placed before "import numpy as np" (wrong-import-order)
tobac/tracking.py:27:0: C0411: standard import "import math" should be placed before "import numpy as np" (wrong-import-order)
tobac/tracking.py:31:0: C0411: third party import "from packaging import version as pkgvsn" should be placed before "from . import utils as tb_utils" (wrong-import-order)
tobac/tracking.py:32:0: C0411: third party import "import trackpy as tp" should be placed before "from . import utils as tb_utils" (wrong-import-order)
tobac/tracking.py:33:0: C0411: standard import "from copy import deepcopy" should be placed before "import numpy as np" (wrong-import-order)
tobac/tracking.py:23:0: W0611: Unused is_ imported from operator (unused-import)
tobac/tracking.py:27:0: W0611: Unused import math (unused-import)
tobac/tracking.py:28:0: W0611: Unused utils imported as tb_utils (unused-import)

------------------------------------------------------------------
Your code has been rated at 7.02/10

[JuliaKukulies]

Would it be possible to add an action that runs pylint and posts the results to the PR and highlight if the score has reduced? That way we could highlight linting issues without it being an immediate barrier to contributing

That is a nice idea. I think it should be fairly easy to run pylint and post the result. I can have a look into whether it would be possible to only post the result the if the score has been reduced and if so, only the lines for the modified/added code.

[JuliaKukulies]

but will we end up with headaches between PEP-8 style and Black style, or are those compatible

black is PEP-8 compatible and I am quite sure that people use pylint and black together, so it should not give us any headaches

[freemansw1]

Resolved by #373