[Sugar-devel] Automated pyflakes checking

[William Orr]

> Walter Bender <mailto:walter.bender at gmail.com>
> Wednesday, May 08, 2013 8:17 AM
> On Wed, May 8, 2013 at 5:31 AM, Daniel Narvaez <dwnarvaez at gmail.com 
> <mailto:dwnarvaez at gmail.com>> wrote:
>
>     On 8 May 2013 06:02, William Orr <will at worrbase.com
>     <mailto:will at worrbase.com>> wrote:
>
>
>         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.
>
>         Pull requests are here:
>         https://github.com/sugarlabs/sugar-toolkit-gtk3/pull/25
>         https://github.com/sugarlabs/sugar/pull/49
>
>         Let me know if I need to make any changes.
>
>
>     Awesome! Thanks for working on this!
>
>
> +1
>
>
>     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?
>
>     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.
>
>
> 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.
>
>
>
>     _______________________________________________
>     Sugar-devel mailing list
>     Sugar-devel at lists.sugarlabs.org
>     <mailto:Sugar-devel at lists.sugarlabs.org>
>     http://lists.sugarlabs.org/listinfo/sugar-devel
>
>
>
>
> -- 
> Walter Bender
> Sugar Labs
> http://www.sugarlabs.org
> _______________________________________________
> Sugar-devel mailing list
> Sugar-devel at lists.sugarlabs.org
> http://lists.sugarlabs.org/listinfo/sugar-devel
> Daniel Narvaez <mailto:dwnarvaez at gmail.com>
> Wednesday, May 08, 2013 5:31 AM
> On 8 May 2013 06:02, William Orr <will at worrbase.com 
> <mailto:will at worrbase.com>> wrote:
>
>
>     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.
>
>     Pull requests are here:
>     https://github.com/sugarlabs/sugar-toolkit-gtk3/pull/25
>     https://github.com/sugarlabs/sugar/pull/49
>
>     Let me know if I need to make any changes.
>
>
> Awesome! Thanks for working on this!
>
> 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?
>
> 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.
> _______________________________________________
> Sugar-devel mailing list
> Sugar-devel at lists.sugarlabs.org
> http://lists.sugarlabs.org/listinfo/sugar-devel
> William Orr <mailto:will at worrbase.com>
> Wednesday, May 08, 2013 12:02 AM
>
>
>
> 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.
>
> Pull requests are here:
> https://github.com/sugarlabs/sugar-toolkit-gtk3/pull/25
> https://github.com/sugarlabs/sugar/pull/49
>
> Let me know if I need to make any changes.
>
> Thanks,
> William Orr
> Daniel Narvaez <mailto:dwnarvaez at gmail.com>
> Tuesday, May 07, 2013 8:39 AM
> Hello,
>
> 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).
>
> I'm planning to do the same with pep8, help with making the code pep8 
> clean would be very appreciated.
>
> -- 
> Daniel Narvaez
> _______________________________________________
> Sugar-devel mailing list
> Sugar-devel at lists.sugarlabs.org
> http://lists.sugarlabs.org/listinfo/sugar-devel

Hello,

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.

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.

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?

Thanks for all the feedback!
William Orr