Walter Bender <mailto:walter.ben...@gmail.com>
Wednesday, May 08, 2013 8:17 AM
On Wed, May 8, 2013 at 5:31 AM, Daniel Narvaez <dwnarv...@gmail.com
<mailto:dwnarv...@gmail.com>> wrote:
On 8 May 2013 06:02, William Orr <w...@worrbase.com
<mailto:w...@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@lists.sugarlabs.org
<mailto:Sugar-devel@lists.sugarlabs.org>
http://lists.sugarlabs.org/listinfo/sugar-devel
--
Walter Bender
Sugar Labs
http://www.sugarlabs.org
_______________________________________________
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel
Daniel Narvaez <mailto:dwnarv...@gmail.com>
Wednesday, May 08, 2013 5:31 AM
On 8 May 2013 06:02, William Orr <w...@worrbase.com
<mailto:w...@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@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel
William Orr <mailto:w...@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:dwnarv...@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@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
_______________________________________________
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel
