flake8 fix

[chimosky]

@quozl @walterbender, line 37 of pysamples/ta-stats.py DIROFINTEREST was mentioned which is not defined but _DIROFINTEREST is defined and is a string which cannot be concatenated with an in, the line could be len(_DIROFINTEREST) + 2.

TurtleArt/taexportlogo.py line 129 has an undefined variable HAS_DATASTORE, what should the test case be?.

TurtleArt/taprimitive.py line 862 has a variable named self.

[quozl]

@chimosky, thanks for the questions.

ta-stats.py

The file pysamples/journal-stats.py is very similar to pysamples/ta-stats.py.

Looking at commits that affected these files, the most recent is 70e9d986cce071a832e76d6f51cc652f2906fc7d which shows how @AlanJAS copied a change from ta-stats.py to journal-stats.py. A copy and paste error. Likely the change was not tested.

Once you fix this mistake, please test. The file pysamples/ta-stats.py is referenced by the sample samples/media-turtle-stats.ta so you should load this sample to test it. Or you could add a unit test somehow.

taexportlogo.py

It will require either reaching into tw.running_sugar, or just place it in an exception handler that assigns another string if the datastore is not reachable. The program is also written to run without Sugar, so you would also have to test that way.

taprimitive.py

I don't know. It looks like the code in __init__ should never be executed, so you can replace that with either pass or a fatal error instead.

[quozl]

Thanks for your patches. Reviewed to 2cc831e.

• [ ] you have saved files added. This is an easy mistake to make, and easy to fix. Please read the diff you are about to commit before you commit! Make sure that every change shown by the diff is what you expect.

• [ ] from gi.repository import GdkX11, GstVideo is critical in tagplay.py, you can't remove it without breaking video playback. See Record activity .flake8 for how to document it.

• [ ] your .flake8 file has ignore lines without explanation; in future please add explanations so they can be reviewed.

• [ ] your .flake8 file has more than one ignore line, so only the last line is applied. See Record activity .flake8 file for how to list more than one ignore.

• [ ] textchannelwrapper.py is substantially CollabWrapper, yet you've not done what you did in https://github.com/sugarlabs/collabwrapper/commit/b02ddc5d477a709c44411111f6678de42522df62.

• [ ] your change to TurtleArt/turtleblocks.py

-                if not name in plugins:
+                if not(name in plugins):

would be better as if name not in plugins.

As there's enough above, I've stopped reviewing, to avoid wall-of-text.

Your branch has 8313 more flake8 issues. Normally at this point I'd ask you to continue, but read on.

This is a bad time to do flake8 on Turtle Art without also doing it on Turtle Confusion and the other two or three derivative repositories. It's bad because it takes Turtle Art and Turtle Confusion further apart, making the divergence greater, making the work required larger. So if you must do flake8 changes, make them on all the repositories at once. If you missed out on this discussion, see the closed pull requests in Turtle Confusion, and watch that repository as well.

@yashagrawal3, do you have any comment?

[yell0wfl4sh]

I agree with @quozl here, since the work on Turtle Confusion will be much useful if we go on to converge Turtle Confusion and Turtle Art, which is the way to go (now/future), this pull request will add more work to that.

[quozl]

@yashagrawal3, thanks.

@chimosky, how would you like to proceed? You might take this patch https://github.com/sugarlabs/turtleart-activity/pull/60/commits/88a2c56d46bc450825a8b16fc73247b382940448 fix the review issues, and make sure it can apply to both Turtle Art and Turtle Confusion.

[chimosky]

@quozl, I'll do that.

[chimosky]

@quozl @walterbender kindly review, errors now at 124 and the all come fromTurtleArt/util/sugariconify.py, how do you suggest i reduce the characters per line in the file?.

[quozl]

@chimosky, based on my discussion with @yashagrawal3 above, and the convergence being done, I'd like to continue to wait a bit longer before applying flake8 fixes. If they are merged now, it makes converging harder.

[chimosky]

Okay.

[quozl]

Thanks. Reviewed to 6920fdd.

• [ ] while E402 is unavoidable, your other ignores of F401, F601, E741, F811 and F841, are all avoidable, except just one instance of F401 (see below), so please work to fix the code, and trim ignore down to a minimum.

• [ ] you still have saved files added to your commits; as if you aren't looking at the commit diffs at all? Please personally review the diffs, and you'll see what I mean.

• [ ] from gi.repository import GdkX11, GstVideo is critical in tagplay.py, because of the prepare-window-handle flow, yet you have removed it; this will break video playback. Add F401 to ignore, but only once this is the only F401.

• [ ] textchannelwrapper.py is a copy of collabwrapper:collabwrapper.py, which has had flake8 fixes done to it already, in addition to other fixes, yet you're only taking in the flake8 fixes; this makes it harder to bring in the other fixes later; instead, don't change this file unless you are changing it in total.

[chimosky]

changes made

• [x] while E402 is unavoidable, your other ignores of F401, F601, E741, F811 and F841, are all avoidable, except just one instance of F401 (see below), so please work to fix the code, and trim ignore down to a minimum.
• [x] you still have saved files added to your commits; as if you aren't looking at the commit diffs at all? Please personally review the diffs, and you'll see what I mean.
• [x] from gi.repository import GdkX11, GstVideo is critical in tagplay.py, because of the prepare-window-handle flow, yet you have removed it; this will break video playback. Add F401 to ignore, but only once this is the only F401.
• [x] textchannelwrapper.py is a copy of collabwrapper:collabwrapper.py, which has had flake8 fixes done to it already, in addition to other fixes, yet you're only taking in the flake8 fixes; this makes it harder to bring in the other fixes later; instead, don't change this file unless you are changing it in total.

Notes In plugins/audio_sensors/audiograb.py error returned is about output variable assigned but never used and it's been used, what should i do?

In pysamples/grecord.py how should i handle F841 error seems grecord.py is never used

It'll be good to know the intended variable names for params in set_margins of TurtleArt/sprites.py

In TurtleArt/tabasics.py color_names_i18n is important for po files it's assigned but never used here so it raises an F841 error

In TurtleArt/tablocks.py it'll also be good to know the intended variable names

In TurtleArt/taexportlogo.py HAS_DATASTORE is used but undefined

In TurtleArt/taplugin.py I'd love to know the intended variable name for l line 89

In TurtleArt/taprimitive.py what do i do about the variable self

In TurtleArt/tasprite_factory.py what is the intended variable name for l in lines 996, 1007, 1015, 1035

In TurtleArt/tatype.py variable self is also assigned

In TurtleArt/tawindow.py line 151 error is F811 redefinition of unused 'activity' from line 43 line 4330 has resp which runs the dialog box but error is thrown F841 local variable resp is assigned to but never used

In TurtleArt/util/codegen.py F405 errors are raised about dictionary keys

In TurtleArt/util/configfile.py I'd like to know the intended variable name for l in lines 100, 109

In TurtleArt/util/configwizard.py I'd like to know the intended variable name for l in line 89

In TurtleArt/util/sugariconify.py how do i escape lines in self.previewHTML line 342

In TurtleArt/util/odf/attrconverters.py how do you suggest i handle the F601 error

In TurtleArt/util/odf/element.py lines 333, 490, 491 and 497 allowed_args and prefix are assigned but never used

In TurtleArt/util/odf/opendocument.py line 549 returns F821 error undefined name cdata and i can't seem to trace name

@quozl @walterbender errors now at 113, kindly review

[quozl]

I'm just not interested yet; this work goes against what I had planned to do, making either my work or your work so much harder.

Here's your git diff --stat;

 .flake8                                       |   7 +
 TurtleArt/sprites.py                          |   2 -
 TurtleArt/sprites.py.save                     | 487 +++++++++++++++++
 TurtleArt/sprites.py.save.1                   | 383 ++++++++++++++
 TurtleArt/sprites.py.save.2                   | 488 ++++++++++++++++++
 TurtleArt/tabasics.py                         |  19 +-
 TurtleArt/tacanvas.py                         |   9 -
 TurtleArt/tacollaboration.py                  |  17 +-
 TurtleArt/taconstants.py                      |   5 +-
 TurtleArt/taexportlogo.py                     |   5 +-
 TurtleArt/tagplay.py                          |   1 +
 TurtleArt/tajail.py                           |   2 -
 TurtleArt/talogo.py                           |   6 +-
 TurtleArt/taprimitive.py                      |  20 +-
 TurtleArt/taturtle.py                         |   2 +-
 TurtleArt/tatype.py                           |   2 +-
 TurtleArt/tautils.py                          |   4 +-
 TurtleArt/tawindow.py                         |  27 +-
 TurtleArt/textchannelwrapper.py               |   3 +-
 TurtleArt/turtleblocks.py                     |  18 +-
 TurtleArt/util/codegen.py                     |   1 -
 TurtleArt/util/odf/__init__.py                |   2 +-
 TurtleArt/util/odf/attrconverters.py          |  79 ++-
 TurtleArt/util/odf/draw.py                    |   2 +-
 TurtleArt/util/odf/element.py                 |  97 ++--
 TurtleArt/util/odf/grammar.py                 | 115 +++--
 TurtleArt/util/odf/manifest.py                |   2 +-
 TurtleArt/util/odf/meta.py                    |   2 +-
 TurtleArt/util/odf/namespaces.py              |   3 +-
 TurtleArt/util/odf/odfmanifest.py             |  11 +-
 TurtleArt/util/odf/office.py                  |   2 +-
 TurtleArt/util/odf/opendocument.py            |  40 +-
 TurtleArt/util/odf/style.py                   |   2 +-
 TurtleArt/util/sugariconify.py                |  12 +-
 TurtleArtActivity.py                          |   4 +-
 gnome_plugins/fb_plugin.py                    |   2 +-
 gnome_plugins/uploader_plugin.py              |   1 -
 plugins/audio_sensors/audiograb.py            |   6 +-
 plugins/camera_sensor/camera_sensor.py        |   7 +-
 plugins/camera_sensor/tacamera.py             |   7 -
 plugins/camera_sensor/v4l2.py                 |  27 +-
 plugins/rfid/rfidrweusb.py                    |   2 -
 plugins/rfid/serial/serialposix.py            |  37 +-
 plugins/rfid/serial/serialutil.py             |  16 +-
 plugins/rfid/tis2000.py                       |   6 +-
 .../turtle_blocks_extras.py                   |  17 +-
 pyexported/window_setup.py                    |   4 -
 pysamples/dotted_line.py                      |   6 +-
 pysamples/grecord.py                          |   1 -
 pysamples/push_time.py                        |   4 +-
 pysamples/ta-stats.py                         |   4 +-
 turtleblocks                                  |  34 --
 turtleblocks.py                               |  34 ++
 53 files changed, 1746 insertions(+), 350 deletions(-)

So you're changing 53 files, and the number of insertions and deletions in each is considerable. This will add many hours of work to converging the several different activities. Or if converging happens first, it will be a similar amount of effort before you can merge this. No serious review yet on https://github.com/sugarlabs/turtleart-activity/pull/63

By the way, your branch diverges from master quite a while back at 4a7df8af8d73884fc7b21c50682e37ec2e0af40a. And you still have saved editor files in your commits; see them in the diff-stat above?

For the moment I'll defer answering your other questions. Good questions though.

[chimosky]

I'll put this on hold until converging is done.

[rhl-bthr]

Stale, closing