Wikimedia Cloud Services team/EnhancementProposals/Decision record T361804 Update python team best practices
Origin task: phab:T361804
Date of the decision: 2024-04-18
No decision meeting was needed, agreement reached in the task.
Decision taken
Option B5 was chosen, to make the wiki page explicitly non-prescriptive and point to a "canonical" example repository.
Page created here.
Rationale
This allows us to still have some variance and freedom to implement different solutions for different repositories by different people but have some common understanding of the current base set of linters/checkers/tools/configs used to keep our codebases somewhat consistent.
Problem
The current best practices page https://wikitech.wikimedia.org/wiki/Wikimedia_Cloud_Services_team/Python_coding does not reflect the practices used in the team maintained repositories (ex. https://gitlab.wikimedia.org/repos/cloud/).
It seems also way more prescriptive that we might want it to be (ex. not explicitly giving space for different configs/special cases/trials).
Those are two different but very related things, that's why they are bundled in one decision request, if we need more space to focus on one or the other, I'll split the request in two.
Constraints and risks
- Tool sprawl
- Overall readability
- Cognitive overhead (all the points are related)
- Style wars
Decision record
In progress
Options
The choices are a combination of A/B and 1/2/3/4/5/...
For example A3 means "Make it prescriptive" and "flake8 + black + isort + 80col + pre-commit...".
While B2 means "Make it explicitly non-prescriptive" and "flake8 + black + 80col"
Prescriptiveness
Option A
Leave it prescriptive, start modifying the current linting rules to match the ones specified in the wiki page.
Pros:
- Homogeneous settings and tooling
Cons:
- Less space for exceptions/special cases/trials/etc.
Option B
Make it explicitly non-prescriptive, and don't enforce/actively change our linting checks to match the wiki page.
This might be done by adding some note like "this is a guideline, not a strict rule, use common sense when applying it"
Pros:
- More space for exceptions/special cases/trials/etc.
Cons:
- Might lead to less homogeneous settings and tooling
Specific tools and settings used
Option 1
Don't change the linting rules, don't change the prescriptiveness (if that is a word), this option is to do absolutely nothing on any side.
Pros:
- Nothing to do on the wiki
Cons:
- Nothing improves and practices and tools continue diverging with time
Option 2
Keep the current mentioned linting rules (flake8 + black + 80 column limit).
Pros:
- All tools synced (when choosing A)
Cons:
- Missing some useful tools/checks
- Using some custom configuration
Option 3
Replace all our check with what the page says (flake8 + black + 80 column limit) and add some extra checkers:
- pre-commit: for the linting/style checks runner
- mypy: static type checking
- isort: import sorting
- trailing-whitespace
- end-of-file-fixer
- check-toml
- check-yaml
- check-shebang-scripts-are-executable
- check-executables-have-shebangs
- check-merge-conflict
Pros:
- All tools synced (when choosing A)
- Nice extra checks
- No custom configuration
Cons:
- None
Option 4
Use the check the current page says, but remove the column limit (use the default for black/isort) and add the extra checks:
- pre-commit: for the linting/style checks runner
- mypy: static type checking
- isort: import sorting
- trailing-whitespace
- end-of-file-fixer
- check-toml
- check-yaml
- check-shebang-scripts-are-executable
- check-executables-have-shebangs
- check-merge-conflict
Pros:
- All tools synced (when choosing A)
- Nice extra checks
- No custom configuration
Cons:
- None
Option 5
Use a fully new set of checkers (using more modern checker [[ https://docs.astral.sh/ruff/ | ruff ]] to replace isort, flake8 and black, and using default column limits). This means using:
- pre-commit: for the linting/style checks runner
- ruff: replaces isort, flake8 and black (and does some static type checking too, not a replacement for mypy)
- mypy: static type checking
- trailing-whitespace
- end-of-file-fixer
- check-toml
- check-yaml
- check-shebang-scripts-are-executable
- check-executables-have-shebangs
- check-merge-conflict
Pros:
- All tools synced (when choosing A)
- Nice extra checks
- No custom configuration
- Modern tooling
Cons:
- None