Add Python 3 compatibility #396

tjstum · 2016-01-10T22:51:42Z

OK, here it is.

17:25:17|stum:[~/gmusicapi] (python3-stage2 *%>)$ tox
...
________________________________________________ summary _________________________________________________
  py27: commands succeeded
  py34: commands succeeded
  py35: commands succeeded
  congratulations :)

The tests work. The example script works. I wasn't able to run the All-Access tests, so if you could help out with that, I'd appreciate it (I'll give it a shot in my application, though).

Please take a look at what I did in setup.py for protobuf. The 3.0.0b2 seems to be working fine, and since this whole thing is a bit experimental to begin with, it seemed OK. I'm happy to do something else, though.

This will need a new release of gpsoauth, with the Python 3 compatibility. I've been manually installing it in each tox environment for now.

I think I incorporated everything from last time, so please let me know what else you'd like me to tackle. Thanks for your patience

simon-weber · 2016-01-11T15:51:52Z

Nice work! Looks like a s/#noqa/# noqa/ will fix most of the linting errors.

It'll take me some time look over the changes, but I should be able to get to it in the next day or two.

tjstum · 2016-01-12T03:54:11Z

OK, I updated the branch to fix the typo in my run of sed. I also fixed the other issues flake8 was complaining about.

simon-weber · 2016-01-13T11:49:52Z

gmusicapi/protocol/mobileclient.py

@@ -185,7 +189,6 @@
        'albums': {'type': 'array', 'items': sj_album, 'required': False},
        'topTracks': {'type': 'array', 'items': sj_track, 'required': False},
        'total_albums': {'type': 'integer', 'required': False},
-        'artistBio': {'type': 'string', 'required': False},


does this field not exist anymore?

It was in the dictionary twice (my editor just flagged it). Look on line 183/187.

good catch. I've been too lazy too hook up pylint 😁

simon-weber · 2016-01-13T12:09:22Z

Nice work! I just have a few small comments; everything else looks good to go.

simon-weber · 2016-01-13T12:12:54Z

setup.py

@@ -42,7 +47,6 @@
        'validictory >= 0.8.0, != 0.9.2',         # error messages
        'decorator >= 3.3.1',                     # > 3.0 likely work, but not on pypi
        'mutagen >= 1.18',                        # EasyID3 module renaming
-        'protobuf >= 2.4.1',                      # 2.3.0 uses ez_setup?
        'requests >= 1.1.0, != 1.2.0, != 2.2.1, < 2.8.0',  # session.close, memory view TypeError
        'python-dateutil >= 1.3, != 2.0',         # 2.0 is python3-only
        'proboscis >= 1.2.5.1',                   # runs_after


I cut gpsoauth 0.1.0 with the compatibility changes (github doesn't let me comment on that line).

It still doesn't pass

- Use set literals - Remove unnecessary list conversions - Convert old_div uses to floor division operator - Fix example.py

- Install the right version of protobuf in setup.py - Describe how tox is supposed to work - Commit tox configuration

tjstum · 2016-01-14T04:40:26Z

I rewrote the branch to remove the modifications to the binaries. That was unintentional.

72cb306 is the commit with changes discussed here.

I appear to have greatly angered travis, though. I think we might want to turn off E402 (module level import not at top of file). I think the problem is that after we do standard_library.install_aliases(), which is required to make other imports do the right thing, any other import counts at "not at the top of the file," which, while technically correct, is kinda missing the point (IMO). I can change setup.cfg to ignore E402 if it sounds OK to you.
If you do ignore that error, you're left with this:

gmusicapi/session.py:36:13: E731 do not assign a lambda expression, use a def
gmusicapi/clients/musicmanager.py:302:9: E731 do not assign a lambda expression, use a def
gmusicapi/protocol/shared.py:55:9: E731 do not assign a lambda expression, use a def
gmusicapi/protocol/shared.py:56:9: E731 do not assign a lambda expression, use a def
gmusicapi/protocol/shared.py:57:9: E731 do not assign a lambda expression, use a def
gmusicapi/protocol/shared.py:58:9: E731 do not assign a lambda expression, use a def

I think all of those are already marked with # noqa, and I didn't touch those things, so I'm not sure how I awoke the beast. Do either of you have more experience with flake8 to point out what happened here?

tjstum · 2016-01-14T04:48:25Z

Oh, also. Do you want me to put the .tox directory in .gitignore? It sort of feels like something that should go in a person's core.excludesfile, but I'm happy to do whichever you think is best.

simon-weber · 2016-01-14T18:25:10Z

Do you want me to put the .tox directory in .gitignore?

I guess we may as well, just in case folks are missing it in their global one.

simon-weber · 2016-01-14T18:28:43Z

I can change setup.cfg to ignore E402 if it sounds OK to you.

👍 sounds good.

I think all of those are already marked with # noqa, and I didn't touch those things, so I'm not sure how I awoke the beast.

I think something bizarre happened with the cron I have set up to rerun the develop build periodically. It looks like it triggered at the right time for it to be that, and travis didn't run on the expected sha.

Let's see what happens when it builds your next push.

simon-weber · 2016-01-16T18:56:01Z

Is there anything else you're looking to address? This looks good to me (the linting errors should go away once we merge).

thebigmunch · 2016-01-16T20:05:12Z

A lot of the list calls wrapping dict methods can be removed to neaten up the code. The futurize script does this indiscriminately for safety, but most instances in gmusicapi are fine without them.

tjstum · 2016-01-16T20:05:37Z

I'll take a look at the small remaining things later tonight. I'm out of
town this weekend.

Do you need me to squash the branch into fewer commits, or is it fine with
you as is?

On Sat, Jan 16, 2016, 11:56 Simon Weber notifications@github.com wrote:

Is there anything else you're looking to address? This looks good to me
(the linting errors should go away once we merge).

—
Reply to this email directly or view it on GitHub
#396 (comment)
.

simon-weber · 2016-01-16T20:07:08Z

Sounds good. Squashing would be great right before we merge (before that point it can make review difficult).

tjstum · 2016-01-17T01:58:08Z

OK, let's see how travis feels after this. (Edit) I don't understand why travis is so upset about this. I can ignore E731? I have no idea what's happening here...

Sounds good. Squashing would be great right before we merge (before that point it can make review difficult).

I was just going to do one commit. Sound fine, @simon-weber?

A lot of the list calls wrapping dict methods can be removed to neaten up the code. The futurize script does this indiscriminately for safety, but most instances in gmusicapi are fine without them.

I cleaned up some of the more gratuitous ones after running futurize. @thebigmunch, if you point me to specific places that you'd like me to remove them from, I will.

simon-weber · 2016-01-17T02:08:21Z

Huh, I can't explain what's going on with travis. It runs on master + your branch, but I don't see anything odd introduced by the merge. Lets not worry about that for now -- if it's still a problem post-merge, hopefully I'll be able to recreate it locally.

Squashing to one commit sounds good 👍

lukegb · 2016-01-17T02:16:14Z

setup.py

 dynamic_requires = []
+# Python 2.7 is supported. Python 3 support is experimental
+if sys.version_info[0] > 2:
+    sys.stderr.write("gmusicapi Python 3 support is experimental.\n")


Instead of doing this, can we raise a warning of some sort instead? That way applications can silence the warning (or display it in a way that's more suitable).

@simon-weber Not sure what your thoughts are on warnings?

Ah, yeah, I like that, and it's backwards compatible with what's done here.

How about warnings.warn("gmusicapi ...", RuntimeWarning)?

thebigmunch · 2016-01-17T16:35:02Z

@tjstum: From a cursory scan, these are the leftovers I spotted that could be changed.

tjstum · 2016-01-18T00:04:02Z

@thebigmunch, I took your changes in ffb7f0e. I didn't take some of them, though, since they looked unsafe to me (the ones that modify the dictionary while iterating over it).

@simon-weber, I made some changes to the testing code to reduce the number of warnings that happen during testing. Please let me know if that sounds OK.

If everyone is happy with the branch as of ffb7f0e, I'll squash it down (I can put it in a separate pull request to keep things clear, if that is preferred)

simon-weber · 2016-01-18T00:05:55Z

lgtm 👍

tjstum · 2016-01-18T00:15:13Z

Squashed and re-opened in #398

tjstum · 2016-01-18T00:21:12Z

Oh, @simon-weber, I'm not sure I made this entirely clear. I wasn't able to run the All-Access tests (but I could run my application, which downloads AA songs). I mentioned that over email a few months ago, but let me know if there are all of a sudden test failures that I can look at.

Sorry for forgetting to mention this sooner!

simon-weber · 2016-01-18T00:24:55Z

No worries -- I ran the tests back when you first let me know and they ran fine. Looks like they just passed on travis too 👍

I'll see if I can recreate the flake8 error.

simon-weber · 2016-01-18T00:32:00Z

Looks like it's a bug in flake8; b43a481 fixes it.

simon-weber · 2016-01-18T00:45:52Z

Apparently this is working as intended (!?). @thebigmunch linked me to https://gitlab.com/pycqa/flake8/issues/21.

tjstum · 2016-01-18T18:41:55Z

Huh. flake8 is so opinionated, I guess

tjstum mentioned this pull request Jan 11, 2016

Add support for Python3 using the python-future library #312

Closed

simon-weber reviewed Jan 13, 2016
View reviewed changes

tjstum added 8 commits January 13, 2016 22:56

Run stage 2 fixes with standard boilerplate

9a5eb80

Get the test suite to run on Python 3

3902a3a

It still doesn't pass

Fix tests to run on Python 2 and 3

2298d80

Minor cleanup

ed3b842

- Use set literals - Remove unnecessary list conversions - Convert old_div uses to floor division operator - Fix example.py

Mark the star imports noqa

5bb8a2a

Mark package as Python 3 compatible

d5e81ca

- Install the right version of protobuf in setup.py - Describe how tox is supposed to work - Commit tox configuration

Fix lint issues

10afe18

CR comments

72cb306

Ignore some non-problematic things

c145945

lukegb reviewed Jan 17, 2016
View reviewed changes

Final fixes

ffb7f0e

tjstum mentioned this pull request Jan 18, 2016

Add support for Python 3 #398

Merged

tjstum closed this Jan 18, 2016

tjstum mentioned this pull request Jan 18, 2016

Request: Python3 support? #247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Python 3 compatibility #396

Add Python 3 compatibility #396

tjstum commented Jan 10, 2016

simon-weber commented Jan 11, 2016

tjstum commented Jan 12, 2016

simon-weber Jan 13, 2016

tjstum Jan 13, 2016

simon-weber Jan 13, 2016

simon-weber commented Jan 13, 2016

simon-weber Jan 13, 2016

tjstum commented Jan 14, 2016

tjstum commented Jan 14, 2016

simon-weber commented Jan 14, 2016

simon-weber commented Jan 14, 2016

simon-weber commented Jan 16, 2016

thebigmunch commented Jan 16, 2016

tjstum commented Jan 16, 2016

simon-weber commented Jan 16, 2016

tjstum commented Jan 17, 2016

simon-weber commented Jan 17, 2016

lukegb Jan 17, 2016

simon-weber Jan 17, 2016

thebigmunch commented Jan 17, 2016

tjstum commented Jan 18, 2016

simon-weber commented Jan 18, 2016

tjstum commented Jan 18, 2016

tjstum commented Jan 18, 2016

simon-weber commented Jan 18, 2016

simon-weber commented Jan 18, 2016

simon-weber commented Jan 18, 2016

tjstum commented Jan 18, 2016

Add Python 3 compatibility #396

Add Python 3 compatibility #396

Conversation

tjstum commented Jan 10, 2016

simon-weber commented Jan 11, 2016

tjstum commented Jan 12, 2016

simon-weber Jan 13, 2016

Choose a reason for hiding this comment

tjstum Jan 13, 2016

Choose a reason for hiding this comment

simon-weber Jan 13, 2016

Choose a reason for hiding this comment

simon-weber commented Jan 13, 2016

simon-weber Jan 13, 2016

Choose a reason for hiding this comment

tjstum commented Jan 14, 2016

tjstum commented Jan 14, 2016

simon-weber commented Jan 14, 2016

simon-weber commented Jan 14, 2016

simon-weber commented Jan 16, 2016

thebigmunch commented Jan 16, 2016

tjstum commented Jan 16, 2016

simon-weber commented Jan 16, 2016

tjstum commented Jan 17, 2016

simon-weber commented Jan 17, 2016

lukegb Jan 17, 2016

Choose a reason for hiding this comment

simon-weber Jan 17, 2016

Choose a reason for hiding this comment

thebigmunch commented Jan 17, 2016

tjstum commented Jan 18, 2016

simon-weber commented Jan 18, 2016

tjstum commented Jan 18, 2016

tjstum commented Jan 18, 2016

simon-weber commented Jan 18, 2016

simon-weber commented Jan 18, 2016

simon-weber commented Jan 18, 2016

tjstum commented Jan 18, 2016