formatting is currently not verified by linter in quarry. This can probably be updated with https://wikitech.wikimedia.org/wiki/Wikimedia_Cloud_Services_team/Python_coding#Formatting
This task is two parts really:
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| Introduce black formatting to quarry | analytics/quarry/web | master | +502 -317 |
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Open | None | T288982 Productionize quarry a bit | |||
| Resolved | rook | T288976 Add black formatting to quarry linter |
We can add a check for formatting with something similar to https://gerrit.wikimedia.org/r/plugins/gitiles/labs/tools/maintain-kubeusers/+/refs/heads/master/tox.ini#30
Since the thing will fail if we add it now, it should probably come with a big formatting patch with some care.
We can copy the scripts from spicerack: https://gerrit.wikimedia.org/r/plugins/gitiles/operations/software/spicerack/+/refs/heads/master/tox.ini#33
That will allow formatting it too (and using isort also), and it's setup already in a way that works on CI.
It formats it in place after submission? Call me old, but I don't like that. We intentionally set up our repos, which also work on CI so that it checks but doesn't format. You can usually set up black to run in text editor if you like that way of doing things. That also is running black without the compromise of 80 char lines that we agreed on.
No, it has a 'check' mode, that is what runs after submission (on ci), but also has a 'format' mode that will format it the way the CI wants (that you have to manually run if you want to).
About the 80 char lines, that's just a config value that can be changed, it's been a while since I cared what that number is xd
That script just runs the command https://gerrit.wikimedia.org/r/plugins/gitiles/operations/software/spicerack/+/refs/heads/master/utils/format-code.sh
You can set up a config (or run it with -l 80). However, my example above already does that with the CLI arg. 80 was the compromise on the team after some objected to 79 and most objected to black's 90 :). Overall, I see no advantage to changing the practices or using that script when we already do this just fine and have a coding standard. I'm objecting to using the spicerack example vs my own example just above it from one of our repos because the main differences are the line length, running in an external script and that it runs without the check arg.
The reason it runs in an external script is the puppet repo convention of only messing with what you changed in git, which isn't needed if you don't run without the check arg and expect the repo to be "clean". No. Let's not do that.
After side chats and double checking things, why don't we put a simpler check that just checks the whole repo in CI after we format, but perhaps also add a tox command to format things too because that would be handy for people who don't have silly plugins for their text editors (and might be useful to volunteers for that reason)?
Change 807534 had a related patch set uploaded (by Vivian Rook; author: Vivian Rook):
[analytics/quarry/web@master] Introduce black formatting to quarry
Change 807534 merged by jenkins-bot:
[analytics/quarry/web@master] Introduce black formatting to quarry