Checking out a new, at least to me, python codebase where the
Cool. Let’s run these commands.
Executed successfully. No warnings. No errors. Thumbs up. On with
Executed successfully. Has reformatted a single file. Cool.
Before the reformat there was the following line. (Think of it as one line even if this blog setup splits it into two)
The reformat did apply the maximum line length rule and changed the above to
Ok cool, LGTM, commit and push.
Seconds later I get back a failed pipeline result.
src/backend/processor.py:335:5: C901 ‘Processor.validate_action’ is too complex (14)
Let’s check C901.
Ok, that’s what the
# noqa C901 comment did.
flake to ignore the configured complexity rules for the
Awesome now I can either:
# noqa: C901comment - All while taking into account the method now being split into three lines
Plus I’m left wondering why there was a file checked into mainline which did not follow the code formatting rules.
Turns out the
black formatting is not part of the pipelines. But
flake8 is. Explains the failure above.
Fine, the pipeline should probably not reformat the code automatically. As a result it would have to create its own reformat commits.
Two additional todos get added to my backlog:
blackhas to format - need to google that - because if there is a reformat
blackwill just reformat and exit with code 0 (success).
Additionally we notice the
README.md states the two commands in the wrong order.
black should have been run first to catch the error before creating a commit.
A third TODO appears.
Now without having fixed the actual issue, the complexity of
validate_action, we have already gathered three additional todos.
We enter the common dilemma on whether to be a good sport and fix these issues or to just not give a f*ck, undo the formatting, push changes and have the green build again.
(And even if we give a f*ck we might want to undo the formatting - to be back to green and then to fix all the above)
We could quickly improve the readability of
validate_action, achieved by extracting the logical parts into methods. Sadly extracting methods does not improve complexity, only readability. Great example for the difference between complexity and being complicated.
Or we could change the rules to increase the max characters per line limit. Or to allow more complexity in methods. Both would have to be discussed with the team.
Quickly removing the types
TestRunAction to shorten the line and have the necessary space to fit
# noqa: C901 there?
Great, making the code worse to game the linter.
We can take the above as an example why forcing formatting rules on a team doesn’t just magically achieve the same formatting everywhere.
flake8 does not automatically improve code quality. It can simply be disabled as it was in the case above. As a result we get cryptic ignore statements throughout our code base that probably no one will ever fix.
So what you are saying is these tools don’t help us?
Yes. I believe so.
Although, f*ck, it depends.
If the team really cares we don’t need a linter. Errors would be caught by tests and formatting would be applied by the editor. Everyone would not want to have their code differ from what is already in the code base.
If the team does not care too deeply these tools only add additional friction. Leading to ignore statements throughout the code. Increased build time plus commits such as “satisfy linter”. And of course a team that hates the rules but does not want to bother with having a discussion about changing them.
Especially if there are folks on the team who peruse and try these rules with the same energy a kid would apply to the candy in a candy store. Have them all.
To add insult to injury - Both tools did not help us to avoid having both snake_case (the
validation_action) and camelCase (the
Probably another rule available in the docs of