Created attachment 210269 [details] [review]
Correct pyflakes warnings/errors

This looks a bit curious:

-    app = EntryBufferApp()
+    EntryBufferApp()

With that there is apparently no reference to the EntryBufferApp object any more, so that it looks like it would be garbage-collected immediately by Python. But demonstrably it works anyway, so I guess it inserts itself into Gtk somehow?

-from ._glib import *
+from ._glib import (GError, IOChannel, IO_ERR, IO_FLAG_APPEND,
+                    IO_FLAG_GET_MASK, IO_FLAG_IS_READABLE, IO_FLAG_IS_SEEKABLE,
+                    IO_FLAG_IS_WRITEABLE, IO_FLAG_MASK, IO_FLAG_NONBLOCK,
[..]

This looks a bit ugly, and would updating with each new symbol. It would also cause incompatibilities with different GTK versions. I think we should rather silence pyflakes at that point, as this is quite deliberate IMHO, you import the _glib symbols into the namespace?

+# Types
+GError = GError
+IOChannel = IOChannel
[...]

What does that do? This is a very long list which looks totally redundant, and just a workaround for pyflakes? Can we disable pyflakes for this part instead?
Same for the similar list in gi/_gobject/__init__.py.

--- a/tests/test_everything.py
+++ b/tests/test_everything.py
@@ -246,7 +246,8 @@ class TestCallbacks(unittest.TestCase):
         """
         def callback():
             x = 1 / 0
-            
+            print x
+

Not Python 3 compatible. I suggest something like

   self.fail('unexpectedly surviving division by zero: ' + str(x))

which would be more useful, and Python 3 compatible.

tests/test_pygtkcompat.py:

-import atk
-import pango
-import pangocairo

Perhaps you could instead add some trivial tests which ensure that one symbol of these modules is available, to at least verify that the namespaces can be imported?

--- a/tests/test_source.py
+++ b/tests/test_source.py
@@ -91,8 +91,8 @@ class TestSource(unittest.TestCase):

 class TestTimeout(unittest.TestCase):
      def test504337(self):
-        timeout_source = GLib.Timeout(20)
-        idle_source = GLib.Idle()
+         GLib.Timeout(20)
+         GLib.Idle()

This could instead do something minimal like

  self.assertNotEqual(GLib.Idle(), None)

or perhaps

  self.assertTrue(hasattr(GLib.Timeout(20), 'id'))

?

Attachment 210269 [details] pushed as c8bc6ae - Correct pyflakes warnings/errors

I pushed before I saw these comments, I'll add a follow up patch with the fixes.

(In reply to comment #2)
> This looks a bit curious:
> 
> -    app = EntryBufferApp()
> +    EntryBufferApp()
> 
> With that there is apparently no reference to the EntryBufferApp object any
> more, so that it looks like it would be garbage-collected immediately by
> Python. But demonstrably it works anyway, so I guess it inserts itself into Gtk
> somehow?

EntryBufferApp, self will have a reference to a Window which is floating and referenced by pygobject.


> 
> -from ._glib import *
> +from ._glib import (GError, IOChannel, IO_ERR, IO_FLAG_APPEND,
> +                    IO_FLAG_GET_MASK, IO_FLAG_IS_READABLE,
> IO_FLAG_IS_SEEKABLE,
> +                    IO_FLAG_IS_WRITEABLE, IO_FLAG_MASK, IO_FLAG_NONBLOCK,
> [..]
> 
> This looks a bit ugly, and would updating with each new symbol. It would also
> cause incompatibilities with different GTK versions. I think we should rather
> silence pyflakes at that point, as this is quite deliberate IMHO, you import
> the _glib symbols into the namespace?
> 

What kind of incompatibilities, _glib defines all symbols unconditionally as far as I can te..



> +# Types
> +GError = GError
> +IOChannel = IOChannel
> [...]
> 
> What does that do? This is a very long list which looks totally redundant, and
> just a workaround for pyflakes? Can we disable pyflakes for this part instead?
> Same for the similar list in gi/_gobject/__init__.py.

It's not completely redudant as it provides a nice and clear list of what's actually exported. For instance when I added the pyflakes checks I could see that we added the same symbol twice, which is something that * imports is hiding.

So even if it weren't for pyflakes I'd think that this change has a positive effect.

> 
> --- a/tests/test_everything.py
> +++ b/tests/test_everything.py
> @@ -246,7 +246,8 @@ class TestCallbacks(unittest.TestCase):
>          """
>          def callback():
>              x = 1 / 0
> -            
> +            print x
> +
> 
> Not Python 3 compatible. I suggest something like
> 
>    self.fail('unexpectedly surviving division by zero: ' + str(x))

I'll change this, thanks for sorting it out.

> -import atk
> -import pango
> -import pangocairo
> 
> Perhaps you could instead add some trivial tests which ensure that one symbol
> of these modules is available, to at least verify that the namespaces can be
> imported?

Yeah, that makes sense and is better.

> 
> --- a/tests/test_source.py
> +++ b/tests/test_source.py
> @@ -91,8 +91,8 @@ class TestSource(unittest.TestCase):
> 
>  class TestTimeout(unittest.TestCase):
>       def test504337(self):
> -        timeout_source = GLib.Timeout(20)
> -        idle_source = GLib.Idle()
> +         GLib.Timeout(20)
> +         GLib.Idle()
> 
> This could instead do something minimal like
> 
>   self.assertNotEqual(GLib.Idle(), None)
> 
> or perhaps
> 
>   self.assertTrue(hasattr(GLib.Timeout(20), 'id'))
> 
> ?

Doesn't really matter imho, we just don't care about the return value, just the fact that the constructor is callable with these arguments.

Created attachment 210271 [details] [review]
Correct review comments from Martin

I'm pushing this so the build is not broken for python 3, but I'll keep this bug open until Martin has a chance to look at it.

(In reply to comment #4)
> > +# Types
> > +GError = GError
> > +IOChannel = IOChannel
> > [...]
> > 
> > What does that do? This is a very long list which looks totally redundant, and
> > just a workaround for pyflakes? Can we disable pyflakes for this part instead?
> > Same for the similar list in gi/_gobject/__init__.py.
> 
> It's not completely redudant as it provides a nice and clear list of what's
> actually exported. For instance when I added the pyflakes checks I could see
> that we added the same symbol twice, which is something that * imports is
> hiding.

But what does that actually do? It looks like a lot of nops. This really just looks like a pyflakes workaround, and cluttering code that much to work around pyflake's deficiencies sounds bad. pylint allows you to override particular errors for particular places in the code, so perhaps we should use that instead?

Thanks for the followup patch. It uses the deprecated failUnless(), but we have a couple more of those, I'll fix them now.

Thanks for fixing this!