Created attachment 319802 [details] [review]
gi.require_version() enhancement-2

Added default values for better compatibility (both backward and forward).

Created attachment 319804 [details] [review]
test for gi.require_version()

Added test for `gi.require_version()`.
Also fixed method body content for `TestImporter.test_require_version_warning()` (the content was duplicated from `TestImporter.test_invalid_repository_module_name()`)

Created attachment 319805 [details] [review]
gi.require_version() enhancement-3

Be more explicit with assigning value of `requires` variable.
Use requires.items() for iteration (for py2/py3 compatible code).

Btw, sorry for all the noise. I'm having "one of those days" :-|

Review of attachment 319805 [details] [review]:

Thanks for the patches. Please see [1] for commit message guidelines.

While I think the feature is nice, I generally dislike overloaded functions. How about adding this as an additional require_versions() function?

[1] https://wiki.gnome.org/Git/CommitMessages

Created attachment 320525 [details] [review]
gi: Add require_versions() function

gi: Add require_versions() function

This new function accepts a dict of one or more namespace, version
pairs. Moves the logic from gi.require_version() to the new
gi.require_versions(), the former now simply calls the latter. Also
adds a new test and fixes an existing test that was broken.

https://bugzilla.gnome.org/show_bug.cgi?id=761141

Just wanted to check in on this. Please let me know your thoughts. Thanks.

Review of attachment 320525 [details] [review]:

Overall this looks good. Just a few noted comments with regards to test dependencies. It also looks like your commit message didn't make it into that patch, are you using "git format-patch" ?

Note that the GNOME 3.20 release is in API freeze so we would need to request a freeze break to get this in (which I think would probably be fine).

Thanks.

::: tests/test_import_machinery.py
@@ +139,3 @@
+        with check("Gdk", 3):
+            with warnings.catch_warnings(record=True) as warning:
+                from gi.repository import Gdk

We need be careful about using repos which are not GLib, GObject, Gio and GIMarshallingTests because the test suite needs to run on a minimal install (Gtk not installed). See [1].

[1] https://git.gnome.org/browse/pygobject/tree/tests/test_overrides_gtk.py#n62

@@ +150,3 @@
+    def test_gi_require_versions(self):
+
+        gi.require_versions({'AppStreamGlib': '1.0', 'Gtk': '3.0', 'GLib': '2.0'})

Same here.

Created attachment 322217 [details] [review]
gi: Add require_versions() function

I incorporated your feedback. I also found and corrected an error in my logic for checking the type of arguments at the start of gi.require_version(). Let me know if you need anything else from me. Thanks.

Just checking in on this again. I'm not sure how much time there is left to get it in for 3.20?

Review of attachment 322217 [details] [review]:

Dustin,

Prior to submitting a freeze break for this I've gone through the code once more and ran tests. There are still a few problems/questions to address (apologies for this taking so long).

To make this addition simpler for reviewers and clearer in terms of functional composition, I suggest leaving gi.require_version() as is and adding the additional gi.require_versions() which simply calls the original in a loop, something like:


    def require_versions(versions):
        for namespace, version in versions.items():
            require_version(namespace, version)


Additionally I'm hitting unittest failures with this:

$ make check TEST_NAMES=test_import_machinery
...
======================================================================
ERROR: test_gi_require_versions (test_import_machinery.TestImporter)
----------------------------------------------------------------------
Traceback (most recent call last):

+ Trace 236011

======================================================================
ERROR: test_require_version_warning (test_import_machinery.TestImporter)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/simon/Source/gnome-3.20/src/pygobject/tests/test_import_machinery.py", line 143, in test_require_version_warning
    self.assertTrue(issubclass(warning[0].category, PyGIWarning))
IndexError: list index out of range

======================================================================
FAIL: test_gi_require_version (test_import_machinery.TestImporter)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/simon/Source/gnome-3.20/src/pygobject/tests/test_import_machinery.py", line 172, in test_gi_require_version
    gi.require_version('GLib', '2.0')
AssertionError: ValueError not raised

::: gi/__init__.py
@@ +95,3 @@
+    Example:
+
+    :type requires: dict

Example should use: gi.require_versions(...)

@@ +103,3 @@
+        raise TypeError(
+            'requires must be of type {0}, {1} given.'.format(type(dict), type(requires))
+        )

Generally avoid strict type checking (this could be removed completely). Or if you really must, test for the abstract collections.Mapping, see:
https://docs.python.org/2/library/collections.html#collections.Mapping

Created attachment 324358 [details] [review]
adds new function gi.require_versions()

Incorporated feedback.

Could I get a status update on this? Thanks.

Comment on attachment 324358 [details] [review]
adds new function gi.require_versions()

This was committed with a fix to a test failure:

    from gi import GLib

was changed to:

    from gi.repository import GLib

See: https://git.gnome.org/browse/pygobject/commit/?id=1bb267

Thanks!