Added proposed Python guidelines to Krita

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



This change is related to

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

R37 Krita
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
victorw created this revision.Jul 24 2017, 10:42 PM

I ran 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"


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)+"    ""-""\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) + "    " + + "-" + + "\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.