Page MenuHomePhabricator

Python3 style guide
Open, MediumPublic

Description

We should agree on some python3 styles and ensure that the CI checks in the puppet repo implement the agreed styles.

To start things I have heard one complaint related to W504 so that would be the first one to consider

If we really hate theses rules, we can rollback https://gerrit.wikimedia.org/r/c/operations/puppet/+/510613 until we have consensus

Details

Related Gerrit Patches:

Event Timeline

jbond created this task.Wed, Nov 27, 1:19 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptWed, Nov 27, 1:19 PM
jbond renamed this task from Python3 style guid to Python3 style guide.Wed, Nov 27, 1:21 PM
jbond updated the task description. (Show Details)
jbond added a subscriber: Volans.
jbond updated the task description. (Show Details)Wed, Nov 27, 1:23 PM
jbond triaged this task as Medium priority.Wed, Nov 27, 1:45 PM
CDanis updated the task description. (Show Details)Wed, Nov 27, 5:23 PM
CDanis added a subscriber: CDanis.
jbond added a comment.EditedWed, Nov 27, 5:25 PM

we should potentially also refresh discussions on black. potentially with a CI hook to automaticity reformat the code via a pre-commit hook

BBlack added a subscriber: BBlack.Wed, Nov 27, 5:33 PM

I will float the opinion that while I may have many opinions on code style, bikeshedding between reasonable options for a shared standard is a waste. If there's a standard upstream/outside-world set of common style rules for python3, we should just adopt them and be done with it.

I don't like the idea of auto-reformatting code in a commit hook, though. We can make sure the formatting tool is easy to run for when one wants to, but I think forcing it as part of the commit process is a bit heavy-handed if CI's going to call out the same things.

I think the best way is to have it easily integrated in some form in the local workflow in our dev envs, so that when you tests locally they passes and when you commit you commit already the formatted version. Then CI is just ensuring that the code is already formatted according to the tool.
Otherwise the resulting workflow would be annoying: make a patch, send it to Gerrit, get V-1 from Jenkins, check the output, either run the tool manually locally or fix manually the format issues, send a second PS.

Change 553487 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] CI - taskgen: add black tests for python2 and python3 files

https://gerrit.wikimedia.org/r/553487

jbond added a comment.Thu, Nov 28, 3:18 PM

The .git directory is not versioned, therefore a git hook is by nature, opt-in. If we go down the route of using black i would suggest creating a git hook that users can install and if/when CI fails we include a message that points users to instructions on how to install the hook.

On a more practice created a patch set to see what this would look like on the CI side and the first issue we would need to fix is the python version. black requires python3.6+ however the jenkins containers are still running stretch i.e. python3.5. We would also need to take this into account for an commit hooks

@jbond on the CI instances you have 3.4, 3.5, 3.6 and 3.7 available although the system one is 3.5. Faidon did the packaging a while ago and if you see the CI jobs of many repos they run all environments from tox.

Change 553502 had a related patch set uploaded (by Jbond; owner: John Bond):
[integration/config@master] operations-puppet: add python3-{4,6,7} to image

https://gerrit.wikimedia.org/r/553502

jbond added a comment.Thu, Nov 28, 4:25 PM

@jbond on the CI instances you have 3.4, 3.5, 3.6 and 3.7 available although the system one is 3.5. Faidon did the packaging a while ago and if you see the CI jobs of many repos they run all environments from tox.

thanks i have created a CR to add theses to the image puppet uses for CI

Change 553502 merged by jenkins-bot:
[integration/config@master] operations-puppet: add python3-{4,6,7} to image

https://gerrit.wikimedia.org/r/553502

Mentioned in SAL (#wikimedia-releng) [2019-12-03T10:45:25Z] <hashar> Building releng/operations-puppet to upgrade tox and ship python3.4 3.6 and 3.7 # T239334

Change 554263 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] Update tox, add python3.6 to operations/puppet

https://gerrit.wikimedia.org/r/554263

Change 554263 merged by jenkins-bot:
[integration/config@master] Update tox, add python3.6 to operations/puppet

https://gerrit.wikimedia.org/r/554263

Change 554270 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] docker: fix cache owner in operations-puppet

https://gerrit.wikimedia.org/r/554270

Change 554270 merged by jenkins-bot:
[integration/config@master] docker: fix cache owner in operations-puppet

https://gerrit.wikimedia.org/r/554270

Change 554271 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] Update puppet job to fix pip cache ownership

https://gerrit.wikimedia.org/r/554271

Change 554271 merged by jenkins-bot:
[integration/config@master] Update puppet job to fix pip cache ownership

https://gerrit.wikimedia.org/r/554271

jbond added a comment.Tue, Dec 3, 1:00 PM

The CR to black is now working and you can see what changes are highlighted in this CI console output

Line length needs to be tweaked to conform with our puppet settings for flake8 I guess.

jbond added a comment.EditedTue, Dec 3, 4:49 PM

Line length needs to be tweaked to conform with our puppet settings for flake8 I guess.

I'm pretty sure the line length configured in the tox.ini file applies to both the python2 and python3 test. Although why this was changed to 100 and if this is correct pre-dates my time and there is not much info in the original commit :)

however if you are seeing different result with python 2 and python3 then that is a bug

Edit: Thinking again you probably meant that black needed to be updated to use 100 char line length (will update)

Here is another update with line length set to 100

jbond added a comment.Tue, Dec 3, 5:06 PM

Edit: Thinking again you probably meant that black needed to be updated to use 100 char line length (will update)
Here is another update with line length set to 100

And here is one without string normalisation