After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 781582 - Throw an error if require_version is not passed a string
Throw an error if require_version is not passed a string
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: general
Git master
Other Linux
: Normal enhancement
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks:
 
 
Reported: 2017-04-21 11:42 UTC by Benjamin Berg
Modified: 2017-04-26 10:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Make sure version information passed to require_version is a string. (1.10 KB, patch)
2017-04-21 11:43 UTC, Benjamin Berg
none Details | Review
Make sure version information passed to require_version is a string. (2.12 KB, patch)
2017-04-24 17:30 UTC, Benjamin Berg
none Details | Review
Ooops. Changed both places to "<= 2" to make it more consistent. (2.12 KB, patch)
2017-04-24 17:41 UTC, Benjamin Berg
committed Details | Review
Make sure version information passed to require_version is a string. (2.12 KB, patch)
2017-04-26 10:01 UTC, Benjamin Berg
committed Details | Review

Description Benjamin Berg 2017-04-21 11:42:55 UTC
I spend some time earlier wondering why gi.require_version("Gtk", 3.0) didn't work. Adding in a simple test that the version is a string and throwing a ValueError otherwise could save some hassle for people in the future.
Comment 1 Benjamin Berg 2017-04-21 11:43:32 UTC
Created attachment 350195 [details] [review]
Make sure version information passed to require_version is a string.

This simply makes it easier for someone to find an error in cases where
a floating point is passed by accident. The test simply checks for the
type to be str which should be good in both python2 and python3 but is
not quite ideal in python2 as it misses unicode strings.
Comment 2 Christoph Reiter (lazka) 2017-04-21 13:29:38 UTC
Review of attachment 350195 [details] [review]:

A test checking for the ValueError would be nice (in test_import_machinery.py for example)

::: gi/__init__.py
@@ +109,3 @@
     repository = Repository.get_default()
 
+    if not isinstance(version, str):

Under Python 2 we should check for basestring as currently both str and unicode work there (unicode can also occur unintentionally if someone uses "from __future__ import unicode_literals" etc.)
Comment 3 Benjamin Berg 2017-04-24 17:30:21 UTC
Created attachment 350316 [details] [review]
Make sure version information passed to require_version is a string.

This simply makes it easier for someone to find an error in cases where
a floating point is passed by accident.
Comment 4 Christoph Reiter (lazka) 2017-04-24 17:34:43 UTC
Review of attachment 350316 [details] [review]:

::: tests/test_import_machinery.py
@@ +152,3 @@
+
+        # Test that unicode strings work in python 2
+        if sys.version_info[0] < 2:

<=
Comment 5 Benjamin Berg 2017-04-24 17:41:09 UTC
Created attachment 350317 [details] [review]
Ooops. Changed both places to "<= 2" to make it more consistent.

Make sure version information passed to require_version is a string.

This simply makes it easier for someone to find an error in cases where
a floating point is passed by accident.
Comment 6 Christoph Reiter (lazka) 2017-04-24 18:38:42 UTC
Review of attachment 350317 [details] [review]:

lgtm
Comment 7 Benjamin Berg 2017-04-26 10:01:23 UTC
The following fix has been pushed:
9cbf370 Make sure version information passed to require_version is a string.
Comment 8 Benjamin Berg 2017-04-26 10:01:31 UTC
Created attachment 350444 [details] [review]
Make sure version information passed to require_version is a string.

This simply makes it easier for someone to find an error in cases where
a floating point is passed by accident.