Going per file, enabling also line too long, is probably the best approach. It's going to take some more time, but better be careful since unit test coverage is almost none.<span></span><br><br>On Wednesday, 8 May 2013, William Orr  wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Walter Bender <mailto:<a>walter.bender@gmail.com</a>><br>
Wednesday, May 08, 2013 8:17 AM<br>
On Wed, May 8, 2013 at 5:31 AM, Daniel Narvaez <<a>dwnarvaez@gmail.com</a> <mailto:<a>dwnarvaez@gmail.com</a>>> wrote:<br>
<br>
    On 8 May 2013 06:02, William Orr <<a>will@worrbase.com</a><br>
    <mailto:<a>will@worrbase.com</a>>> wrote:<br>
<br>
<br>
        I've done some initial work on both sugar and<br>
        sugar-toolkit-gtk3 with respect to PEP8. I've also added<br>
        support for PEP8 in make check for both packages.<br>
<br>
        Pull requests are here:<br>
        <a href="https://github.com/sugarlabs/sugar-toolkit-gtk3/pull/25" target="_blank">https://github.com/sugarlabs/<u></u>sugar-toolkit-gtk3/pull/25</a><br>
        <a href="https://github.com/sugarlabs/sugar/pull/49" target="_blank">https://github.com/sugarlabs/<u></u>sugar/pull/49</a><br>
<br>
        Let me know if I need to make any changes.<br>
<br>
<br>
    Awesome! Thanks for working on this!<br>
<br>
<br>
+1<br>
<br>
<br>
    I didn't go through all the changes yet, but I have one general<br>
    concern. You have disabled E501 (line too long), which would be<br>
    fine as an intermediate step, though you seem to be regressing<br>
    line length when fixing the other issues. Perhaps it make sense to<br>
    do the E501 changes together so that we avoid regressions?<br>
<br>
    If you end up reworking the patches it might also make sense to<br>
    split them up a bit more (even arbitrarily) just to review/land<br>
    more gradually and diminish a bit the risk of conflicts etc. I<br>
    don't expect the review to be particularly complicated, though<br>
    there are a few odd cases with line continuations that we will<br>
    need to check/discuss/tweak.<br>
<br>
<br>
Also, there are a few non-pep8 fixes mixed in as well, e.g., replacing type() == with isinstance()... a needed change but perhaps the kind of thing that could go in a separate patch.<br>
<br>
<br>
<br>
    ______________________________<u></u>_________________<br>
    Sugar-devel mailing list<br>
    <a>Sugar-devel@lists.sugarlabs.org</a><br>
    <mailto:<a>Sugar-devel@lists.sugarlabs.org</a>><br>
    <a href="http://lists.sugarlabs.org/listinfo/sugar-devel" target="_blank">http://lists.sugarlabs.org/<u></u>listinfo/sugar-devel</a><br>
<br>
<br>
<br>
<br>
-- <br>
Walter Bender<br>
Sugar Labs<br>
<a href="http://www.sugarlabs.org" target="_blank">http://www.sugarlabs.org</a><br>
______________________________<u></u>_________________<br>
Sugar-devel mailing list<br>
<a>Sugar-devel@lists.sugarlabs.org</a><br>
<a href="http://lists.sugarlabs.org/listinfo/sugar-devel" target="_blank">http://lists.sugarlabs.org/<u></u>listinfo/sugar-devel</a><br>
Daniel Narvaez <mailto:<a>dwnarvaez@gmail.com</a>><br>
Wednesday, May 08, 2013 5:31 AM<br>
On 8 May 2013 06:02, William Orr <<a>will@worrbase.com</a> <mailto:<a>will@worrbase.com</a>>> wrote:<br>
<br>
<br>
    I've done some initial work on both sugar and sugar-toolkit-gtk3<br>
    with respect to PEP8. I've also added support for PEP8 in make<br>
    check for both packages.<br>
<br>
    Pull requests are here:<br>
    <a href="https://github.com/sugarlabs/sugar-toolkit-gtk3/pull/25" target="_blank">https://github.com/sugarlabs/<u></u>sugar-toolkit-gtk3/pull/25</a><br>
    <a href="https://github.com/sugarlabs/sugar/pull/49" target="_blank">https://github.com/sugarlabs/<u></u>sugar/pull/49</a><br>
<br>
    Let me know if I need to make any changes.<br>
<br>
<br>
Awesome! Thanks for working on this!<br>
<br>
I didn't go through all the changes yet, but I have one general concern. You have disabled E501 (line too long), which would be fine as an intermediate step, though you seem to be regressing line length when fixing the other issues. Perhaps it make sense to do the E501 changes together so that we avoid regressions?<br>

<br>
If you end up reworking the patches it might also make sense to split them up a bit more (even arbitrarily) just to review/land more gradually and diminish a bit the risk of conflicts etc. I don't expect the review to be particularly complicated, though there are a few odd cases with line continuations that we will need to check/discuss/tweak.<br>

______________________________<u></u>_________________<br>
Sugar-devel mailing list<br>
<a>Sugar-devel@lists.sugarlabs.org</a><br>
<a href="http://lists.sugarlabs.org/listinfo/sugar-devel" target="_blank">http://lists.sugarlabs.org/<u></u>listinfo/sugar-devel</a><br>
William Orr <mailto:<a>will@worrbase.com</a>><br>
Wednesday, May 08, 2013 12:02 AM<br>
<br>
<br>
<br>
I've done some initial work on both sugar and sugar-toolkit-gtk3 with respect to PEP8. I've also added support for PEP8 in make check for both packages.<br>
<br>
Pull requests are here:<br>
<a href="https://github.com/sugarlabs/sugar-toolkit-gtk3/pull/25" target="_blank">https://github.com/sugarlabs/<u></u>sugar-toolkit-gtk3/pull/25</a><br>
<a href="https://github.com/sugarlabs/sugar/pull/49" target="_blank">https://github.com/sugarlabs/<u></u>sugar/pull/49</a><br>
<br>
Let me know if I need to make any changes.<br>
<br>
Thanks,<br>
William Orr<br>
Daniel Narvaez <mailto:<a>dwnarvaez@gmail.com</a>><br>
Tuesday, May 07, 2013 8:39 AM<br>
Hello,<br>
<br>
we are now automatically checking sugar and sugar-toolkit-gtk3 using pyflakes. Please make sure your patches are pyflakes clean, so that we don't break the build. (Running "make check" before pushing is a good way to ensure it).<br>

<br>
I'm planning to do the same with pep8, help with making the code pep8 clean would be very appreciated.<br>
<br>
-- <br>
Daniel Narvaez<br>
______________________________<u></u>_________________<br>
Sugar-devel mailing list<br>
<a>Sugar-devel@lists.sugarlabs.org</a><br>
<a href="http://lists.sugarlabs.org/listinfo/sugar-devel" target="_blank">http://lists.sugarlabs.org/<u></u>listinfo/sugar-devel</a><br>
</blockquote>
<br>
Hello,<br>
<br>
Obviously, before submitting I ran the unit tests and did a make run and performed basic tasks. I know this doesn't uncover all errors, but I just wanted you all to know I wasn't just submitting untested code. Provided unit test coverage is good (I haven't looked at the unit test suites yet), everything is probably kosher.<br>

<br>
Regarding the logic changes, those are reported as pep8 errors according the pep8 tool, which is why they were changed. I did my best to preserve the original meaning, as well as to include them as a separate (second) commit.<br>

<br>
One thing I can do is go back and make my changes per file and commit those one at a time, would you prefer that?<br>
<br>
Thanks for all the feedback!<br>
William Orr<br>
</blockquote><br><br>-- <br>Daniel Narvaez<br><br>