Page MenuHomePhabricator

PEP-8 compliance
Closed, ResolvedPublic

Description

Before officially releasing 2.0 all code (especially the library) should be PEP-8 compliant and non-compliant code should be marked deprecated. But I also think that this shouldn't be rushed as this gives us a chance to change the method headers for example and improve them without the need to deprecate parts of the new PEP-8 method header.

So my suggestion would be the following (prior to release pywikibot 2.0):

  • If the method is changed but can be easily done, it shouldn't be changed to a PEP-8 compliant name
  • If it is very complex (e.g. parameters are removed or repurposed) it should be changed to a PEP-8 compliant name
  • More or less immediately before the release we change all names to PEP-8 compliant names and deprecate the old names (this doesn't apply to already changed methods obviously). This shouldn't be done minutes prior to the release obviously but with one of the last steps. It shouldn't be rushed but thought out so that the names are flexible enough that they don't need to be deprecated.

Non PEP-8 compliant names are a very easy way to get away with failures in the API, because the PEP-8 compliant names can be easily added without concern about backwards compatibility, because the old name can translate to the new name. This also means that we should be careful with premature renaming. For example APISite.hasExtension was already changed to APISite.has_extension but there has been suggestions to make has_extension more like hasExtension and so any advantage would be lost and our final has_extension needs to be compatible with the current has_extension.

Event Timeline

XZise created this task.Dec 25 2014, 9:30 PM
XZise assigned this task to jayvdb.
XZise raised the priority of this task from to Needs Triage.
XZise updated the task description. (Show Details)
XZise added a project: Pywikibot.
XZise added subscribers: Unknown Object (MLST), jayvdb, XZise, Xqt.

I agree with the 2.0 library being 'mostly'-PEP8-compliant for the official API, but dont care about scripts (which may not even be packaged in the pywikibot package) and also dont mind if private names (esp. variable names), and argument names are not all compliant.

By 'mostly', I mean the spirit of PEP8 should be complied with, but not the letter of the style guide. e.g. APISite has lots of method names which are comprised of two workds. e.g. unwantedpages. That should be split with an underscore, but doing that is going to create a lot of code churn, new bugs, etc. We currently have scripts which do not work at all , scripts which fail spectacularly when certain args are used, or fail with used on a wiki other than the home wiki of the script writer, usually because an i18n message only exists in one language.

XZise added a comment.Jan 3 2015, 2:25 PM

Well everything we don't deprecate until 2.0 will be also in 3.0 so still in 3.0 some names would need to be guessed. Is it get_redirect_target, getredirecttarget or getRedirectTarget? And as soon as getRedirectTarget is deprecated it just could call get_redirect_target and if that call fails at some point because of an update, get_redirect_target has been broken anyway.

I can agree with private stuff, because those aren't in the official API so renaming them shouldn't be a problem. With scripts it depends if they are packaged, but even when I don't have a problem if those aren't compliant. And I might be okay with some names if they are directly connected with the API. But apart from that it should be consistent, including the arguments because there is the same guessing problem. For example Page.getVersionHistory has a parameter reverseOrder, so there we could fix both in one step: Just deprecate the method and call the newly added method get_version_history which now has reverse (or reverse_order).

XZise added a comment.Jan 3 2015, 10:51 PM

Also another thing is that the MW API might change. So a parameter like fromrev may might sense now (because it's what the API is using) but maybe MW is changing it to source and it doesn't make sense anymore. So having methods named like the API isn't such a good idea after all.

Ricordisamoa added a subscriber: Ricordisamoa.

Another reason to break away from MW API names is those names will be less relevant when we supporting non-MW wikis.

XZise updated the task description. (Show Details)Jan 15 2015, 12:41 PM
XZise set Security to None.

Another reason to break away from MW API names is those names will be less relevant when we supporting non-MW wikis.

Seriously, are we going to support them? More or less four people are developing the framework...

XZise added a comment.Jan 15 2015, 8:59 PM

But even if not, the MW API is not static (see query continue) and thus it's unreasonable to have names which might not make sense anymore in the future. Additionally for someone who doesn't understand how api.php works, this will also confuse further, and what happens if an argument is not directly mapped to something on the API, should that now follow PEP-8 or not?

Huji added a subscriber: Huji.Jan 6 2017, 4:56 PM

Question for the team: I check PEP8 compliance using http://pep8online.com/ and one of the rules it enforces is that the file must end with a newline (i.e. one blank line at the very end is required). I cannot find that recommendation on python.org's PEP8 documentation but it is mentioned on online forums so I guess it is/was part of PEP8.

Our scripts in pywikibot do not comply with this. Should we consider complying?

Magul added a subscriber: Magul.EditedJan 6 2017, 6:26 PM

Hi @Huji I don't remember if it is actually pointed out in original PEP-8 document but it was introduced in pycodestyle (formerly known as pep8, changed on direct Guido's requests) as warning W292.

My linter in editor point this issue every time and as I don't see any argument agains I would go with fulfilling with it, but it's not such big problem to treat it as urgent matter, it could be fixed during other changes.

Huji added a comment.Jan 6 2017, 6:43 PM

@Magul I also agree that we shouldn't do like one big commit that only consists of adding blank lines to the end of files.

But perhaps to ensure that this change actually does happen over time, can we modify our test scripts on Jenkins to enforce this rule? This way, every time someone submits a patch for a file that does not end with a blank line, they will receive a -1 from Jenkins, and they will fix it. And once that goes into effect, the next person wouldn't get the warning. Over time, as all files are modified, they all become compliant.

On that note, do we even run a full PEP8 test on all patches? From what I see, currently four test scripts are run on every patch, called pywikibot-core-tox-jessie, pywikibot-core-tox-nose-jessie, pywikibot-core-tox-nose34-jessie and pywikibot-core-tox-doc-jessie. Of these, the last three don't seem to do any PEP8 related checks. The first one does something called PEP8-naming, and I haven't looked into it but I guess it only enforces rules regarding function names and variable names and class names and such.

If my assessment is correct, should we expand our tests to include the full PEP8 specifications?

Magul added a comment.Jan 7 2017, 3:11 PM

@Huji I'm aware that we are running 4 jobs to verify any gerrit patch, but haven't found time to check how they are defined. There is also plan to move the whole testing procedure to Wikimedia CI infrastructure (see task T132138), so if You have time to update our jobs there that will be really big step in right direction.

Huji added a comment.Jan 8 2017, 1:25 AM

@Magul I am not familiar with Wikimedia CI so I am afraid I cannot help. What I can tell you for sure is that we do not check PEP8 compliance right now; I checked a number of our existing scripts and they failed for reasons like having lines longer than 80 characters, having import commands not at the top of the file, etc.

Xqt triaged this task as Lowest priority.May 28 2017, 2:21 PM
Dvorapa changed the status of subtask T87409: pep8 E402 vs __version__ and others from Resolved to Declined.Jan 29 2018, 10:18 AM
Xqt changed the status of subtask T87409: pep8 E402 vs __version__ and others from Declined to Resolved.Feb 2 2018, 6:28 AM
xSavitar moved this task from Backlog to Needs Review on the Pywikibot board.Nov 5 2018, 11:37 AM
Xqt removed jayvdb as the assignee of this task.Nov 9 2018, 4:16 PM
Huji added a comment.Aug 26 2019, 6:06 PM

@Xqt from what I can see, the only issue pycodestyle finds with our current scripts is lack of compliance with E731 (do not assign a lambda expression, use a def) which happens with only the following:

  • pywikibot/site.py
  • scripts/flickrripper.py
  • scripts/interwiki.py

How about we refactor those three (I think I can do that) and add pycodestyle to the CI jobs for pywikibot so that it is enforced prospectively?

Huji added a comment.EditedAug 26 2019, 10:20 PM

Once https://gerrit.wikimedia.org/r/#/c/pywikibot/core/+/532473/ is merged, we are all clear (given the exclusions we have specified in tox.ini of course) and we can close this task. CI already takes care of the rest.

Unless we want to go crazy and try to mitigate some of the other noqa cases. For instance, in pywikibot/data/api.py we have import cPickle as pickle which is merely for convenience and violates N813 but I feel like these deliberate exceptions should be untouched and we should still call the code PEP-8 compliant despite the noqa's.

Xqt closed this task as Resolved.Aug 27 2019, 4:32 AM