Hello there!
At first, I am really happy to see the pywikipedia framework is still alive and very active.
As you may have noticed, whenever a change is send to Gerrit that triggers Jenkins jobs that run the code style utilities pep8 and pyflakes.
It seems your code is not passing the style checks so whenever they fail it is not going to prevent you from merging the code.
I am not sure how your community likes pep8/pyflakes. But I think it would be nice to have the code repositories to pass those tests and enforce authors to follow them. You can still have some pep8 checks ignored such as the "line too long".
The questions are:
Is there any interest in making your repositories pep8 compliants? If so, is there anything I can do to help? :-]
Le 29/07/13 13:30, Antoine Musso a écrit :
You can still have some pep8 checks ignored such as the "line too long".
As an example, say you want to get rid of the "line too long error". When running pep8 you would get a message like:
$ pep8 . ./foo.py:1:80: E501 line too long (105 > 79 characters) $
E501 is pep8 error code. To have it ignored, create a file named '.pep8' at the root of your repository and fill it with:
[pep8] # Ignore E501: line too long ignore = E501
The line beginning with a # is a comment. You can ignore more code by using comma (ie: ignore = E501, E666).
When running pep8 again:
$ pep8 . $
Success! Lines too long are entirely ignored.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 29.07.2013 13:30, Antoine Musso wrote:
Hello there!
At first, I am really happy to see the pywikipedia framework is still alive and very active.
As you may have noticed, whenever a change is send to Gerrit that triggers Jenkins jobs that run the code style utilities pep8 and pyflakes.
It seems your code is not passing the style checks so whenever they fail it is not going to prevent you from merging the code.
I am not sure how your community likes pep8/pyflakes. But I think it would be nice to have the code repositories to pass those tests and enforce authors to follow them. You can still have some pep8 checks ignored such as the "line too long".
The questions are:
Is there any interest in making your repositories pep8 compliants? If so, is there anything I can do to help? :-]
In fact you are right and PEP8 is a good thing, regarding pyflakes I do not know this yet. I heard of "pylint" also.
As I see is xqt very active in PEP8 code style changes, so I would advice you to contact him. In general I assume nobody within this community has a strong oppinion against proper code style... ;)
Greetings DrTrigon
I am against strictly enforced rules, altough generally I think PEP8 a good think and I committed some PEP8 changes, too. Xqt is the main agent here.
However, *sometimes *breking the rules helps maintain readability, and breaking the rules when it is neccessary is one of Wikipedia's main rules. :-) An example for long lines: URL in comment where the problem or solution is explained.
pep8 allows marking specific lines as 'I am breaking the rules here -- ignore this line' by adding # noqa at the end of the line.
On 29 July 2013 19:11, Bináris wikiposta@gmail.com wrote:
I am against strictly enforced rules, altough generally I think PEP8 a good think and I committed some PEP8 changes, too. Xqt is the main agent here.
However, *sometimes *breking the rules helps maintain readability, and breaking the rules when it is neccessary is one of Wikipedia's main rules. :-) An example for long lines: URL in comment where the problem or solution is explained.
-- Bináris _______________________________________________ Pywikipedia-l mailing list Pywikipedia-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/pywikipedia-l
Le 29/07/13 19:20, Merlijn van Deen a écrit :
pep8 allows marking specific lines as 'I am breaking the rules here -- ignore this line' by adding # noqa at the end of the line.
I dont think the '# noqa' thing supported by pep8 v1.3.3 which is running on the Jenkins server :-(
On 29 July 2013 21:13, Antoine Musso hashar+wmf@free.fr wrote:
I dont think the '# noqa' thing supported by pep8 v1.3.3 which is running on the Jenkins server :-(
Hm, indeed. # noqa is only supported in v1.4.1+; # nopep8 was introduced in v1.4+, and thus is also not available in 1.3. Would it be possible to upgrade to a more recent version? Ubuntu Saucy has 1.4.5 available.
Merlijn
Le 29/07/13 21:19, Merlijn van Deen a écrit :
Hm, indeed. # noqa is only supported in v1.4.1+; # nopep8 was introduced in v1.4+, and thus is also not available in 1.3. Would it be possible to upgrade to a more recent version? Ubuntu Saucy has 1.4.5 available.
Merlijn
I apparently managed to backport pep8 1.4.5 from Saucy to Precise using the tutorial at:
https://wikitech.wikimedia.org/wiki/Backport_packages
I have installed the package on the instance and it seems to be running fine. I have filled https://bugzilla.wikimedia.org/52239 to record the log of the build and track the backporting itself.
I will have to announce the upgrade ahead of time since it will introduce new failures that might makes some repos to suddenly fail their pep8 check.
Hi folks,
is there any ability now to ignore pep8 or pyflake testing from a specific code line?
Greetings
xqt
----- Original Nachricht ---- Von: Antoine Musso hashar+wmf@free.fr An: pywikipedia-l@lists.wikimedia.org Datum: 29.07.2013 21:41 Betreff: Re: [Pywikipedia-l] python styling (pep8/pyflakes)
Le 29/07/13 21:19, Merlijn van Deen a écrit :
Hm, indeed. # noqa is only supported in v1.4.1+; # nopep8 was introduced in v1.4+, and thus is also not available in 1.3. Would it be possible to upgrade to a more recent version? Ubuntu Saucy has 1.4.5 available.
Merlijn
I apparently managed to backport pep8 1.4.5 from Saucy to Precise using the tutorial at:
https://wikitech.wikimedia.org/wiki/Backport_packages
I have installed the package on the instance and it seems to be running fine. I have filled https://bugzilla.wikimedia.org/52239 to record the log of the build and track the backporting itself.
I will have to announce the upgrade ahead of time since it will introduce new failures that might makes some repos to suddenly fail their pep8 check.
-- Antoine "hashar" Musso
Pywikipedia-l mailing list Pywikipedia-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/pywikipedia-l
Some of them (I think mainly the line breaking ones) can be ignored by adding # noqa to the end of the line. If we really need something more than that, we can look at a custom solution. What was the line that was giving you trouble?
Merlijn
On 3 November 2013 13:30, info@gno.de wrote:
Hi folks,
is there any ability now to ignore pep8 or pyflake testing from a specific code line?
Greetings
xqt
----- Original Nachricht ---- Von: Antoine Musso hashar+wmf@free.fr An: pywikipedia-l@lists.wikimedia.org Datum: 29.07.2013 21:41 Betreff: Re: [Pywikipedia-l] python styling (pep8/pyflakes)
Le 29/07/13 21:19, Merlijn van Deen a écrit :
Hm, indeed. # noqa is only supported in v1.4.1+; # nopep8 was
introduced
in v1.4+, and thus is also not available in 1.3. Would it be possible
to
upgrade to a more recent version? Ubuntu Saucy has 1.4.5 available.
Merlijn
I apparently managed to backport pep8 1.4.5 from Saucy to Precise using the tutorial at:
https://wikitech.wikimedia.org/wiki/Backport_packages
I have installed the package on the instance and it seems to be running fine. I have filled https://bugzilla.wikimedia.org/52239 to record the log of the build and track the backporting itself.
I will have to announce the upgrade ahead of time since it will introduce new failures that might makes some repos to suddenly fail their pep8 check.
-- Antoine "hashar" Musso
Pywikipedia-l mailing list Pywikipedia-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/pywikipedia-l
Pywikipedia-l mailing list Pywikipedia-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/pywikipedia-l
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
However, /sometimes /breking the rules helps maintain readability, and breaking the rules when it is neccessary is one of Wikipedia's main rules. :-) An example for long lines: URL in comment where the problem or solution is explained.
I fine with this and do not see why this should be a problem. As Antoine and Merljin explained breaking the rules ca be done on a global level for any rule wishes as well as on a specific case based level with # noqa. And I see it exactly as you do and would like to underline it with PEP20:
[...] Special cases aren't special enough to break the rules. Although practicality beats purity. [...]
To make it clear; I vote for automated style checks in order to act for us (the developers - humans) as an additional help and source of info (e.g. about the code "quality" or "conformity"). I AM STRICTLY AGAINST failing style checks to prevent us from merging the code! So the bot should run and inform us - as it does right now - and Antoine can help/support e.g. xqt with PEP8 changes as done in the past. This includes all changes that improve code readability, maintainability and do some good - but we should (of course) NOT insist in PEP8 for all kinds of code pieces and stuff like comments, docu and else. And NOT change how the bot behaves right now (e.g. block merges).
Furthermore this PEP8 "transition" or compilance check should go gradually, e.g.: 1.) we exclude ALL PEP8 rules violated by our current code in the '.pep8' file 2.) we take the first rule excluded and try to solve all issues 3.) we remove this rule exclusion from '.pep8' file 4.) we iterate 2.) to 3.) until all issues possible are solved 5.) we check what we have, if and how to proceed
Greetings DrTrigon
On 29 July 2013 20:29, Dr. Trigon dr.trigon@surfeu.ch wrote:
To make it clear; I vote for automated style checks in order to act for us (the developers - humans) as an additional help and source of info (e.g. about the code "quality" or "conformity"). I AM STRICTLY AGAINST failing style checks to prevent us from merging the code!
That would make the bot completely useless, as code that does not pass causes pep8 to fail could enter the repository and thus make all further patches /also/ fail the test.
We *should* set the bot to voting. If there is a special case that does not pass pep8, either decide whether it is a general case (and disable that pep8 warning) or whether it is a local case (and add # noqa to the end of the line). In both cases, the bot will tell you the code passes validation (because, well, you told it to ignore the non-valid part), and the change can be merged.
Merlijn
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
That would make the bot completely useless, as code that does not pass
I don't think so.
On 30 July 2013 00:08, Dr. Trigon dr.trigon@surfeu.ch wrote:
I don't think so.
I would be interested to understand how. Because pep8 validation works *on the entire repository* and not just on *the patchset, *the following is what I'm afraid will happen:
1) all code passes pep8 validation 2) someone uploads a patchset that adds a mistake: the merged repository does not pass validation 3) the bot reports the patchset failed validation 4) the change is merged -> the repository no longer passes validation!
And because the repository no longer passes validation, the bot will also report 'failure' on any following patchset!
As such, the repository needs to always pass validation. To make sure the repository always passes validation, no changes should be merged if they fail validation. The easiest way to prevent the changes to be merged is to set the bot to voting.
Merlijn
Le 30/07/13 10:52, Merlijn van Deen a écrit :
On 30 July 2013 00:08, Dr. Trigon <dr.trigon@surfeu.ch mailto:dr.trigon@surfeu.ch> wrote:
I don't think so.I would be interested to understand how. Because pep8 validation works /on the entire repository/ and not just on /the patchset, /the following is what I'm afraid will happen:
- all code passes pep8 validation
- someone uploads a patchset that adds a mistake: the merged repository
does not pass validation 3) the bot reports the patchset failed validation 4) the change is merged -> the repository no longer passes validation!
And because the repository no longer passes validation, the bot will also report 'failure' on any following patchset!
As such, the repository needs to always pass validation. To make sure the repository always passes validation, no changes should be merged if they fail validation. The easiest way to prevent the changes to be merged is to set the bot to voting.
That is exactly how it "should" work if one want to prevent new style error from entering the repository. Note that I am not enforcing the pywikipediabot developers one way or another :-]
What I would recommend is: - make the repositories to pass the pep8 / pyflakes tests - enable blocking
After a few days, you will get a good idea of style errors you do not care about. pep8 style is sometime annoying:
Given the code:
somemodule.longmethod.chaining.there(p='foo', anotherpara=('value which can potentially be super long'))
pep8 yields:
E128 continuation line under-indented for visual indent
One can make it pass with the very ugly:
somemodule.chaining.there(p='foo', anotherpara=('value which can potentially' 'be super long'))
Or putting each parameter on each on line:
somemodule.longmethod.chaining.there( p='foo', anotherpara=('value which can potentially be super long'))
That specific E128 code can be ignored with a .pep8 file containing:
[pep8] # This is a commented out line # E128 continuation line under-indented for visual indent ignore = E128
So once you experimented pep8 annoyance, you can selectively get rid of some checks :-]
Another issue, is people sending their patch without checking the style locally. The best way to handle this is to use a code editor that triggers pep8 on file save and reports back to you straight in the code editor. I am sure IDE dedicated to python already support that.
For vim there is the syntastic plugin: https://github.com/scrooloose/syntastic (screenshot included)
That is a huge time saver.
cheers,
I use sublime and I found this https://github.com/tanelpuhu/sublime-text-2-packages/tree/master/Python%20PE...
Sublime is so much better than other editors in programming. I really recommend you to test it
Best
On 7/30/13, Antoine Musso hashar+wmf@free.fr wrote:
Le 30/07/13 10:52, Merlijn van Deen a écrit :
On 30 July 2013 00:08, Dr. Trigon <dr.trigon@surfeu.ch mailto:dr.trigon@surfeu.ch> wrote:
I don't think so.I would be interested to understand how. Because pep8 validation works /on the entire repository/ and not just on /the patchset, /the following is what I'm afraid will happen:
- all code passes pep8 validation
- someone uploads a patchset that adds a mistake: the merged repository
does not pass validation 3) the bot reports the patchset failed validation 4) the change is merged -> the repository no longer passes validation!
And because the repository no longer passes validation, the bot will also report 'failure' on any following patchset!
As such, the repository needs to always pass validation. To make sure the repository always passes validation, no changes should be merged if they fail validation. The easiest way to prevent the changes to be merged is to set the bot to voting.
That is exactly how it "should" work if one want to prevent new style error from entering the repository. Note that I am not enforcing the pywikipediabot developers one way or another :-]
What I would recommend is:
- make the repositories to pass the pep8 / pyflakes tests
- enable blocking
After a few days, you will get a good idea of style errors you do not care about. pep8 style is sometime annoying:
Given the code:
somemodule.longmethod.chaining.there(p='foo', anotherpara=('value which can potentially be super long'))
pep8 yields:
E128 continuation line under-indented for visual indent
One can make it pass with the very ugly:
somemodule.chaining.there(p='foo', anotherpara=('value which can potentially' 'be super long'))
Or putting each parameter on each on line:
somemodule.longmethod.chaining.there( p='foo', anotherpara=('value which can potentially be super long'))
That specific E128 code can be ignored with a .pep8 file containing:
[pep8] # This is a commented out line # E128 continuation line under-indented for visual indent ignore = E128
So once you experimented pep8 annoyance, you can selectively get rid of some checks :-]
Another issue, is people sending their patch without checking the style locally. The best way to handle this is to use a code editor that triggers pep8 on file save and reports back to you straight in the code editor. I am sure IDE dedicated to python already support that.
For vim there is the syntastic plugin: https://github.com/scrooloose/syntastic (screenshot included)
That is a huge time saver.
cheers,
-- Antoine "hashar" Musso
Pywikipedia-l mailing list Pywikipedia-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/pywikipedia-l
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
One can make it pass with the very ugly:
somemodule.chaining.there(p='foo', anotherpara=('value which can potentially' 'be super long'))
Yes this IS VERY UGLY.
Or putting each parameter on each on line:
somemodule.longmethod.chaining.there( p='foo', anotherpara=('value which can potentially be super long'))
UGLY too. At least the first parameter should be allowed to be on the first line.
That specific E128 code can be ignored with a .pep8 file containing:
[pep8] # This is a commented out line # E128 continuation line under-indented for visual indent ignore = E128
So once you experimented pep8 annoyance, you can selectively get rid of some checks :-]
As I mentioned - IMHO I would start by ignoring everything that our code does not pass at the moment and then starting to improve the code and successively remove ignores whenever possible. The most important style checks are already done or enforced by the python interpreter. (rember the differences between python and e.g. perl)
Also I would like to remember:
http://www.mediawiki.org/wiki/Gerrit/Code_review#Goals
"Tolerate idiosyncrasies where possible. Developers should feel that their creative input is valued and preserved."
Sometimes it's just not possible to decide which style is the better one and I am always intressted in seeing other styles also in order to learn something new (may be better ;) - and avoid to get bored. But of course pep8 is always good for orientation and as "how it SHOULD be".
Greetings to all! DrTrigon
On 31 July 2013 12:03, Dr. Trigon dr.trigon@surfeu.ch wrote:
As I mentioned - IMHO I would start by ignoring everything that our code does not pass at the moment and then starting to improve the code and successively remove ignores whenever possible.
My experience with fixing several files was that it's actually better to walk through the existing code, fixing pep8's complaints or disabling checks, depending on whether the check improves the code or not.
http://www.mediawiki.org/wiki/Gerrit/Code_review#Goals
"Tolerate idiosyncrasies where possible. Developers should feel that their creative input is valued and preserved."
This is very true. But remember having a consistent style also has a big advantage: it makes reading the code easier because of the consistency.
Thanks for your input on the issue.
Merlijn
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Merljin I did not want to criticize you I just don't agree with "That would make the bot completely useless" I find it useful as it is already! As mentioned it would be good to have pep8 but I would be careful with FORCING it by blocking non-pep8-compilant patches/changes because first of all that would just increase the workload to us.
- all code passes pep8 validation 2) someone uploads a patchset
that adds a mistake: the merged repository does not pass validation 3) the bot reports the patchset failed validation 4) the change is merged -> the repository no longer passes validation!
Yes that's of course what can (and according to Murphy also will) happen, but I would just mark susch a commit as bug or at least "to be improved" and fix it. I mean nothing will break if the code is not pep8. It will still work. It is just not that clean and pure anymore.
And because the repository no longer passes validation, the bot will also report 'failure' on any following patchset!
There it would be nice if the bot could be patched in order to become such smart that it can mark this as follow-up to the buggy patchset that was not pep8.
As such, the repository needs to always pass validation. To make sure the repository always passes validation, no changes should be merged if they fail validation. The easiest way to prevent the changes to be merged is to set the bot to voting.
IMHO this is what should be done for unittests strictly but for pep8 it should be tolerant. unittests say that the code does not work. pep8 just says that the code might be ugly. First is strict but second not.
Greetings and thanks a lot for your effort on this!! DrTrigon
Hi Dr. Trigon,
On 31 July 2013 11:53, Dr. Trigon dr.trigon@surfeu.ch wrote:
I find it useful as it is already!
I'm confused to how, exactly :-) Yes, you can now run pep8 locally, which is useful, but the bot just reports 'oh, hey, the code doesn't pass validation', which does not actually tell use something new.
As mentioned it would be good to have pep8 but I would be careful with FORCING it by blocking non-pep8-compilant patches/changes because first of all that would just increase the workload to us.
This is true, although I think this is not too bad. I pep8-ified several files a week ago, and it was not a huge effort, so just fixing a single (or a few) problem in a patchset shouldn't be too much hassle.
- all code passes pep8 validation 2) someone uploads a patchset
that adds a mistake: the merged repository does not pass validation 3) the bot reports the patchset failed validation 4) the change is merged -> the repository no longer passes validation!
Yes that's of course what can (and according to Murphy also will) happen, but I would just mark susch a commit as bug or at least "to be improved" and fix it.
What is the advantage of 'merge first, then (maybe, if you don't forget) fix' over 'fix first, then merge'? It has to be fixed at some point anyway.
I mean nothing will break if the code is not pep8. It will still work. It is just not that clean and pure anymore.
True.
And because the repository no longer passes validation, the bot will also report 'failure' on any following patchset!
There it would be nice if the bot could be patched in order to become such smart that it can mark this as follow-up to the buggy patchset that was not pep8.
I don't understand this point. You mean the bot could mark the fixing patchset as related to the buggy patchset?
Merlijn
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10.08.2013 15:30, Merlijn van Deen wrote:
Hi Dr. Trigon,
On 31 July 2013 11:53, Dr. Trigon <dr.trigon@surfeu.ch mailto:dr.trigon@surfeu.ch> wrote:
I find it useful as it is already!
I'm confused to how, exactly :-) Yes, you can now run pep8 locally, which is useful, but the bot just reports 'oh, hey, the code doesn't pass validation', which does not actually tell use something new.
I assumed (my misstake ;) that the bot just checks the code contained in the changeset/patch not the whole repo. First would be useful indeed since it saves me some work, second would be indeed useless since... yes you are right, that would be nothing new nor useful to know... ;)
As mentioned it would be good to have pep8 but I would be careful with FORCING it by blocking non-pep8-compilant patches/changes because first of all that would just increase the workload to us.
This is true, although I think this is not too bad. I pep8-ified several files a week ago, and it was not a huge effort, so just fixing a single (or a few) problem in a patchset shouldn't be too much hassle.
There are 2 situations; if you have time and are anyway doing several code changes, then you are right that should not be too much hassle. But if you do not have spare time but need to fix an urgent bug - than pep8 might by just very annoying...
- all code passes pep8 validation 2) someone uploads a patchset
that adds a mistake: the merged repository does not pass validation 3) the bot reports the patchset failed validation 4) the change is merged -> the repository no longer passes validation!
Yes that's of course what can (and according to Murphy also will) happen, but I would just mark susch a commit as bug or at least "to be improved" and fix it.
What is the advantage of 'merge first, then (maybe, if you don't forget) fix' over 'fix first, then merge'? It has to be fixed at some point anyway.
Sorry that might be a missunderstanding; of course if you SEE a code or styling but/issue you should (have ;) to fix it directly. I was taking of a bug or issue you introduced with a change accidentially.
And because the repository no longer passes validation, the bot will also report 'failure' on any following patchset!
There it would be nice if the bot could be patched in order to become such smart that it can mark this as follow-up to the buggy patchset that was not pep8.
I don't understand this point. You mean the bot could mark the fixing patchset as related to the buggy patchset?
If you have change 'A' that is not pep8 compillant, the bot should mark it. You are free to improve it (if you DO you are a hero - if not, don't bother...). If it was NOT improved but merged and later another change 'B' (changing the same code as 'A' did) is made which IS pep8 but the whole code IS NOT - then it would be nice if the bot could mention, that the pep8 issue was triggered by change 'A' and not 'B'... something like that, but I think this will be very complex to implement and I am not sure of how much help this will be - was just an idea... :)
Greetings DrTrigon