Added proposed Python guidelines to Krita
ClosedPublic

Authored by victorw on Jul 24 2017, 10:42 PM.

Details

Summary

This change is related to https://phabricator.kde.org/T6588

Here's a proposal for Krita's Python code guidelines to help reduce future headaches. The purpose of this review is to discuss this proposal. Are these too strict, or too vague? Should I spend more time explaining why the rules should be followed? Feel free to reject or suggest changes to any of it, or all of it.

I'm more than happy to discuss the pros and cons of any of these rules! The important thing here is to reach a collective agreement.

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
victorw created this revision.Jul 24 2017, 10:42 PM

I ran autopep8.py on the entire code base as an experiment, and there is one set of PEP8 rules which I'm a bit torn about: The spacing after commas and operators. If it's followed, then

(
"and",       "del",       "for",       "is",        "raise",
"assert",    "elif",      "from",      "lambda",    "return",
"break",     "else",      "global",    "not",       "try",
"class",     "except",    "if",        "or",        "while",
"continue",  "exec",      "import",    "pass",      "yield",
"def",       "finally",   "in",        "print"
)

Turns into

(
    "and", "del", "for", "is", "raise",
    "assert", "elif", "from", "lambda", "return",
    "break", "else", "global", "not", "try",
    "class", "except", "if", "or", "while",
    "continue", "exec", "import", "pass", "yield",
    "def", "finally", "in", "print"
)

And

red   = max(min(int(color.componentsOrdered()[0]*255), 255), 0)
green = max(min(int(color.componentsOrdered()[1]*255), 255), 0)
blue  = max(min(int(color.componentsOrdered()[2]*255), 255), 0)
gplFile.write(str(red)+" "+str(green)+" "+str(blue)+"    "+entry.id+"-"+entry.name+"\n")

turns into

red = max(min(int(color.componentsOrdered()[0] * 255), 255), 0)
green = max(min(int(color.componentsOrdered()[1] * 255), 255), 0)
blue = max(min(int(color.componentsOrdered()[2] * 255), 255), 0)
gplFile.write(str(red) + " " + str(green) + " " + str(blue) + "    " + entry.id + "-" + entry.name + "\n")

They are separate but similar rules, so we can ignore one of them, or both. The reason I'm torn about is because it can make the code a bit harder to read; spotting typos when words align is much easier. On the other hand, in Python code with lots of different modules it can be difficult to find where in the code a variable is assigned to. One way to do it would be to search through all code in the project for "green =", but that might not catch every assignment if this rule is not enforced.

AFAIK there are separate error codes for "one space before =" and "one space after =", so we could enforce "one space before" and allow multiple spaces after it. This would permit lines to sync up, while still enabling global searches of e.g. "green =". Both points here have merits, so I don't have any strong opinions here.

As for the first example, we don't have to mangle the list like that; we could also have every element on a new line. This wastes horizontal space, but can also make diffs cleaner as you can more easily tell which element got added, removed or modified. It also reduces the risk for merge conflicts.

As an aside: Because of the way autopep8 works, it is easier to start out with lots of exceptions and then gradually whitelist them.

woltherav accepted this revision.Aug 30 2017, 10:39 AM

I think this ought to be accepted by now.

This revision is now accepted and ready to land.Aug 30 2017, 10:39 AM
rempt accepted this revision.Feb 28 2018, 1:54 PM

This was accepted, so please push it?

This revision was automatically updated to reflect the committed changes.