Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Full Python 3 compatibility #61

Merged
merged 1 commit into from
Oct 24, 2017
Merged

Conversation

GuillaumeDerval
Copy link
Contributor

I spent some hours to make PyLTI pass the tests again with Python 3 (tested with 2.7, 3.4 and 3.5, I'm not sure it will pass with Python 3.1, 3.2 and 3.3 since they don't support the u prefix from what I remember)

I dropped oauth, since oauth2 is a fork, but oauth2 itself has some problems, which I had to fix manually (see classes SignatureMethod_HMAC_SHA1_Unicode, SignatureMethod_PLAINTEXT_Unicode, Request_Fix_Duplicate) (joestump/python-oauth2#207 joestump/python-oauth2#197).

It fixes #58 as a bonus too.

100% of the test passes with both python 2.7, 3.4 and 3.5, and the lib was tested on edX with Python 3.5, it works flawlessly.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.3%) to 97.698% when pulling 08377bd on GuillaumeDerval:py3-restore into 3266843 on mitodl:master.

@pdpinch
Copy link
Member

pdpinch commented Aug 5, 2016

@amir-qayyum-khan (or maybe @giocalitri if you have some downtime) can you look at this?

@giocalitri
Copy link

I will take a look

@giocalitri giocalitri self-assigned this Aug 5, 2016
from urlparse import urlparse
from urllib import urlencode
except ImportError: # Python 3
from urllib.parse import urlparse, urlencode

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use the six library for these imports

@giocalitri
Copy link

@GuillaumeDerval thanks a lot for your contribution, we really appreciated.

I had some minor comments that actually made me think I should probably fix some other minor things in this project eventually.

Thanks again.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.6%) to 98.442% when pulling 047ae47 on GuillaumeDerval:py3-restore into 3266843 on mitodl:master.

@GuillaumeDerval
Copy link
Contributor Author

I applied most of your comments, and updated the README :-)

Would you like that I squash the commits together?

@giocalitri
Copy link

Thanks for the changes.
Yes, please squash.
We will need to make some tests,so the actual merge can take some time.

@pdpinch
Copy link
Member

pdpinch commented Aug 14, 2016

@giocalitri I took a look at MIT-licensed code (here and here) and it's compatible with our BSD license. It's fine to merge from a licensing perspective.

I still need to find a good test case.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.6%) to 98.442% when pulling 506b25e on GuillaumeDerval:py3-restore into 3266843 on mitodl:master.

@GuillaumeDerval
Copy link
Contributor Author

Any news on this? It would be nice a py3 compatible version on pip :-)

@pdpinch
Copy link
Member

pdpinch commented Aug 2, 2017

@giocalitri can we merge & release this?

@pdpinch pdpinch assigned pdpinch and unassigned giocalitri Aug 7, 2017
@perllaghu
Copy link

I was about to raise an issue to address this very issue - do we have any movement on this?

@pdpinch
Copy link
Member

pdpinch commented Aug 29, 2017

It's passed code review, but we haven't had time to do a functional test. Would you mind giving this branch a try and letting us know if it works for you?

@perllaghu
Copy link

perllaghu commented Aug 30, 2017

It installs just fine in a virtualenv, however it's not installing into a docker container:

Create a Dockerfile in an otherwise empty directory

FROM ubuntu:16.04

RUN apt-get update && apt-get install -y \
  git \
  python3.5 \
  python3-pip

# Load Python modules from requirements.txt
RUN pip3 install git+https:/GuillaumeDerval/pylti@py3-restore#egg=PyLTI

then run
docker build .

produces an error:

Collecting PyLTI from git+https:/GuillaumeDerval/pylti@py3-restore#egg=PyLTI
  Cloning https:/GuillaumeDerval/pylti (to py3-restore) to /tmp/pip-build-homdxv96/PyLTI
    Complete output from command python setup.py egg_info:
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/tmp/pip-build-homdxv96/PyLTI/setup.py", line 121, in <module>
        README = open('README.rst').read()
      File "/usr/lib/python3.5/encodings/ascii.py", line 26, in decode
        return codecs.ascii_decode(input, self.errors)[0]
    UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 2423: ordinal not in range(128)
    
    ----------------------------------------
Command "python setup.py egg_info" failed with error code 1 in /tmp/pip-build-homdxv96/PyLTI/
You are using pip version 8.1.1, however version 9.0.1 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
The command '/bin/sh -c pip3 install git+https:/GuillaumeDerval/pylti@py3-restore#egg=PyLTI' returned a non-zero code: 1

@perllaghu
Copy link

We needed to add

RUN locale-gen en_GB.UTF-8
ENV LC_ALL en_GB.UTF-8

to the Dockerfile, and then we could add git+https:/GuillaumeDerval/pylti@py3-restore#egg=PyLTI to the requirements.txt file for the Django app

The app installs, and django-lti-provider installs fine.

I've not yet tested any LTI functionality though.

@nikolas
Copy link
Contributor

nikolas commented Oct 23, 2017

I'm trying to use this library in Python 3 - can someone merge this and make a new release?

@nikolas
Copy link
Contributor

nikolas commented Oct 23, 2017

FYI - until this is merged and released, I've forked pylti with these changes and made a new release here: https:/ccnmtl/pylti/releases/tag/0.5.0

@pdpinch
Copy link
Member

pdpinch commented Oct 23, 2017

I'm sorry that this PR has been neglected @nikolas. Were you able to use this branch as is, or did you have to make further changes?

@nikolas
Copy link
Contributor

nikolas commented Oct 23, 2017

No problem, it happens. Yeah, this branch is working as is for me, if that helps.

@giocalitri
Copy link

I am going to merge this.
Thanks @GuillaumeDerval for the contribution and everybody for the patience.

@giocalitri giocalitri merged commit d720482 into mitodl:master Oct 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PyLTI needs does not compile with pytest >= 2.8.0
6 participants