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.

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).

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.

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

In T85328#978852, @jayvdb wrote:

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...

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?

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?

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.

@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?

@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.

@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 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?

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.