GNOME Bugzilla – Bug 737643
Port invest-applet to Python 3
Last modified: 2014-10-13 17:05:04 UTC
Created attachment 287419 [details] [review] invest-applet: Port to Python 3 Here is the set of patches to port invest-applet to Python 3. I also fixed some minor unrelated issues with the code while trying to make it work.
Created attachment 287420 [details] [review] configure.ac: Require Python 3
Created attachment 287421 [details] [review] invest-applet: Remove unused code
Review of attachment 287419 [details] [review]: I did not test it yet. ::: invest-applet/invest/chart.py @@ +1,1 @@ #!/usr/bin/python Why you did not change here? ::: invest-applet/invest/invest-applet.py @@ +1,1 @@ +#!/usr/bin/env python3 Should not it be /usr/bin/python3? https://git.gnome.org/browse/gnome-applets/commit/invest-applet/invest/invest-applet.py?id=b3aa1382c80f0ccc62a6ffe98577f95a2c2aea31 ::: invest-applet/invest/invest-chart @@ +1,1 @@ +#!/usr/bin/env python3 Same thing. ::: invest-applet/invest/test.py @@ +1,1 @@ +#!/usr/bin/env python3 Same thing.
Review of attachment 287420 [details] [review]: ok.
Review of attachment 287421 [details] [review]: ok.
Created attachment 287435 [details] [review] invest-applet: Remove unused shebang "/usr/bin/env python" is usually used to not hardcode the interpreter path, as people may have a custom interpreter in /usr/local/bin or virtualenv, which they want to use. chart.py does not need shebang at all, because it is not executable file. I added a patch to remove it.
(In reply to comment #6) > "/usr/bin/env python" is usually used to not hardcode the interpreter path, as > people may have a custom interpreter in /usr/local/bin or virtualenv, which > they want to use. Please use recommended/preferred way. Both debian and fedora suggests to use /usr/bin/python. https://bugzilla.gnome.org/show_bug.cgi?id=692764
Review of attachment 287435 [details] [review]: ok.
> Configure now checks for Python 3, but one of the checks still uses > Python 2 syntax for some reason (but we can probably ignore that). What check?
Created attachment 287456 [details] [review] invest-applet: Port to Python 3 Ok, updated patch.
Created attachment 287459 [details] [review] configure.ac: Require Python 3 > What check? This one: https://git.gnome.org/browse/gnome-applets/tree/m4/python.m4#n46 In Python 3, 'print(foo)' is used instead of 'print foo' (the former works in both 2 and 3). However, we don't need Python headers at all — they are only needed if one wants to write Python extensions in C, and we don't need that. Updated the patch to remove that check.
Applied all patches for testing. Looks like it requires more work... :( 1) Looks like "proxies = invest.PROXY" should be replaced with simply "invest.PROXY" in urlopen function in chart.py and quotes.py. 2) Sorting does not work. I got this error "Failed to populate quotes: 'dict_items' object has no attribute 'sort'". 415 line - quote_items.sort () in quotes.py
Created attachment 288398 [details] [review] [PATCH 1/4] invest-applet: Remove unused imports
Created attachment 288399 [details] [review] [PATCH 2/4] invest-applet: Remove unused code
Created attachment 288400 [details] [review] [PATCH 3/4] invest-applet: Fix some undefined variables in quotes.py
Created attachment 288401 [details] [review] [PATCH 4/4] invest-applet: Port to Python 3 Attached updated patch set. This time it designed in a way that after applying first three patches the applet still works with Python 2 (i.e. it should be safe to do that), while all porting is done in the fourth patch. The code that did sorting was not used, so I dropped it.
Ok. Looks like it is working. Patches added to master.