Flake8 #12

ayushnawal · 2020-01-29T13:30:46Z

Tested.

getting this warning but for svg;
./game.py:517:15: E128 continuation line under-indented for visual indent

resolving this warning leads to:
./game.py:488:80: E501 line too long (105 > 79 characters)

so increasing max line length from 79 to 105

ready to merge :)

srevinsaju

@ayushnawal got more flake8 errors,

./game.py:117:73: W504 line break after binary operator
./game.py:118:71: W504 line break after binary operator
./game.py:391:28: W504 line break after binary operator
./game.py:393:47: W504 line break after binary operator
./game.py:403:28: W504 line break after binary operator
./game.py:404:28: W504 line break after binary operator

I fixed one of them by changing

                xoffset = int((self._width - THIRTEEN * (self._dot_size +
                                                         self._space) -
                               self._space) / 2.)

to

                xoffset = int(
                    (self._width - THIRTEEN * (self._dot_size + self._space) - self._space) / 2.)

ayushnawal · 2020-01-29T18:33:26Z

@srevinsaju I am not getting any error.

i used flake8 .

srevinsaju · 2020-01-29T18:34:21Z

Me too, I am using flake8, version 3.7.9 (mccabe: 0.6.1, pycodestyle: 2.5.0, pyflakes: 2.1.1) CPython 3.8.1 on Linux

PS:
try
flake8 instead of flake8 ., but it should make no difference though

ayushnawal · 2020-01-29T19:02:08Z

flake8 version I was using 3.5.0 (mccabe: 0.6.1, pycodestyle: 2.3.1, pyflakes: 1.6.0) CPython 3.6.9 on Linux

I upgraded it now, got those errors too, fixed.

@srevinsaju please review

srevinsaju · 2020-01-29T19:43:03Z

Yay, good to go, no errors.

quozl · 2020-01-29T19:45:55Z

Thanks, but no. Our maximum line length is the default, about 80 columns. If the code can't be readable after reformatting to match that, don't hide it by changing the maximum line length, just don't change the code.

ayushnawal · 2020-02-05T15:05:17Z

removed max-line-length from .flake8 file as suggested.

no warning except E501 line too long, moreover this occurs in code for svg so can be ignored.

@quozl

quozl · 2020-02-06T00:31:38Z

Rebased on master and added review commits. I felt that many of the changes were excessive, and as it turned out from testing with flake8 many of them were unnecessary. I can't see why you made those changes, as unnecessary changes make git blame noisier and git bisect harder.

I also did not approve causing E501 line to long as a consequence of your changes, so I changed the indentation of the code in game.py that you had changed.

I've more to say, which I'll do in code context of a review.

quozl

Detailed review of aggregate change.

quozl · 2020-02-06T00:32:54Z

game.py

-           for dot in self._dots:
-               dot.set_label(':)')
-           return True
+                new_dot + CIRCLE[c][5][0] + THIRTEEN * CIRCLE[c][5][1]].type:


It would aid comprehension if this line matched the indentation of the other five lines above that start with new_dot, but I did not find a way to do that.

quozl · 2020-02-06T00:33:30Z

game.py

@@ -361,7 +359,7 @@ def _my_strategy_import(self, f, arg):
            self._set_label('Python overflow error: {}'.format(e))
        except TypeError as e:
            self._set_label('Python type error: {}'.format(e))
-        except:
+        except BaseException:


We should avoid BaseException if there is a specific exception we should be handling instead. Is there a specific exception expected?

quozl · 2020-02-06T00:34:51Z

genxo.py

-        return "%s%s%s%s%s%f%s%s%s" % (" style=\"fill:", self._fill, ";stroke:",
-                                       self._stroke, ";stroke-width:",
-                                       self._stroke_width, ";", extras,
+        return "%s%s%s%s%s%f%s%s%s" % (" style=\"fill:",


This separation of each argument into a separate line seems excessive.

quozl · 2020-02-06T00:36:13Z

genxo.py

                                       "\" />\n")

    def _svg_xo(self):
        self.set_stroke_width(3.5)
-        svg_string = "<path d=\"M33.233,35.1l10.102,10.1c0.752,0.75,1.217,1.783,1.217,2.932   c0,2.287-1.855,4.143-4.146,4.143c-1.145,0-2.178-0.463-2.932-1.211L27.372,40.961l-10.1,10.1c-0.75,0.75-1.787,1.211-2.934,1.211   c-2.284,0-4.143-1.854-4.143-4.141c0-1.146,0.465-2.184,1.212-2.934l10.104-10.102L11.409,24.995   c-0.747-0.748-1.212-1.785-1.212-2.93c0-2.289,1.854-4.146,4.146-4.146c1.143,0,2.18,0.465,2.93,1.214l10.099,10.102l10.102-10.103   c0.754-0.749,1.787-1.214,2.934-1.214c2.289,0,4.146,1.856,4.146,4.145c0,1.146-0.467,2.18-1.217,2.932L33.233,35.1z\""
+        svg_string = "<path d=\"M33.233,35.1l10.102,10.1c0.752,0.75,"


This constant had natural separations in the data where there are extra spaces, but those separations have been scattered across the reformatted code.

quozl · 2020-02-06T00:37:36Z

sprites.py

@@ -260,9 +260,9 @@ def set_label(self, new_label, i=0):
            self.labels[i] = str(new_label)
        self.inval()

-    def set_margins(self, l=0, t=0, r=0, b=0):
+    def set_margins(self, w=0, t=0, r=0, b=0):


"l" used to mean left edge. "w" doesn't seem to mean left edge. Same file is in several other activities, so you might check for how this was resolved elsewhere.

quozl · 2020-02-06T00:38:21Z

sprites.py

@@ -352,7 +352,7 @@ def draw(self, cr=None):
                             self.rect[3])
                cr.fill()
            else:
-                print('sprite.draw: source not a pixbuf ({})'.format(type(img)))
+                print('sprite.draw: source not pixbuf ({})'.format(type(img)))


There was nothing wrong with the original error text; I don't think it should be changed in this way.

quozl · 2020-02-06T00:41:18Z

toolbar_utils.py

@@ -152,7 +151,7 @@ def image_factory(image, toolbar, tooltip=None):
 def spin_factory(default, min, max, callback, toolbar):
    spin_adj = Gtk.Adjustment(default, min, max, 1, 32, 0)
    spin = Gtk.SpinButton(spin_adj, 0, 0)
-    spin_id = spin.connect('value-changed', callback)
+    # spin_id = spin.connect('value-changed', callback)


This change is inexplicable given the purpose of the pull request. I note that spin_factory isn't used in this activity, and the source file is used in at least 15 other activities, and spin_factory is used by only one.

quozl · 2020-02-06T00:41:44Z

utils.py

@@ -23,7 +23,7 @@
        import simplejson as json
        from simplejson import load as jload
        from simplejson import dump as jdump
-    except:
+    except BaseException:


A better name for the import failure exception can be obtained by failing an import interactively.

ayushnawal requested a review from tchx84 January 29, 2020 13:43

srevinsaju reviewed Jan 29, 2020

View reviewed changes

ayushnawal and others added 5 commits February 6, 2020 11:20

add .flake8

efe694e

flake8

36d1f7d

remove max line length from .flake8

5729f94

Review - minimise change

b916180

Review - undo line add, my error

05ac383

quozl requested changes Feb 6, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flake8 #12

Flake8 #12

ayushnawal commented Jan 29, 2020 •

edited

Loading

srevinsaju left a comment

ayushnawal commented Jan 29, 2020

srevinsaju commented Jan 29, 2020 •

edited

Loading

ayushnawal commented Jan 29, 2020 •

edited

Loading

srevinsaju commented Jan 29, 2020

quozl commented Jan 29, 2020

ayushnawal commented Feb 5, 2020

quozl commented Feb 6, 2020

quozl left a comment

quozl Feb 6, 2020

quozl Feb 6, 2020

quozl Feb 6, 2020

quozl Feb 6, 2020

quozl Feb 6, 2020

quozl Feb 6, 2020

quozl Feb 6, 2020

quozl Feb 6, 2020

Flake8 #12

Are you sure you want to change the base?

Flake8 #12

Conversation

ayushnawal commented Jan 29, 2020 • edited Loading

srevinsaju left a comment

Choose a reason for hiding this comment

ayushnawal commented Jan 29, 2020

srevinsaju commented Jan 29, 2020 • edited Loading

ayushnawal commented Jan 29, 2020 • edited Loading

srevinsaju commented Jan 29, 2020

quozl commented Jan 29, 2020

ayushnawal commented Feb 5, 2020

quozl commented Feb 6, 2020

quozl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayushnawal commented Jan 29, 2020 •

edited

Loading

srevinsaju commented Jan 29, 2020 •

edited

Loading

ayushnawal commented Jan 29, 2020 •

edited

Loading