Transition from 2to3 to use of six.py to be merged in 5.0.0-dev by joernhees · Pull Request #519 · RDFLib/rdflib (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits78 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

related to and closes #438 but compares to branch 5.0.0-dev instead of master

The old porting approach in #374 to six became quite messy because it started by deactivating 2to3 and then tried to fix all errors. Instead let's try to work through a diff of py2 with py3 after 2to3 and deactivate 2to3 one by one whenever a file is done. In the process the tests are meant to always pass.

Branch is unstable!

Looking at some of the files you already did Jörn, I guess we are happy leaving in u"blah" strings. They are allowed in some later py3, so I would just not support any py3 where they are not ok :)

Jörn, this is pretty much done. How to proceed?

I would maybe squash merge into the 5.0.0 branch, so it's all one commit.

hmm, i'm against squashing, as a single huge commit is always harder to merge than individual ones on upcoming conflicts, etc... except for a lying cleaner and shorter history, what's the benefit?

the types variable became an iterable and was exhausted on 2nd check in py3

So I've converted the rest and removed all special py3 handling from setup et al.

However, py3compat has grown ridicolously - I think having this: https://github.com/RDFLib/rdflib/blob/six_2to3/rdflib/py3compat.py#L18-L52

is weird, when we use six anyway. I'll do a global search replace and import these straight from six in every place.

That way we don't import everything but the kitchen sink for every rdflib module.

Choose a reason for hiding this comment

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

this is bad: if any of the requirements listed above isn't present it will fail... at build time we don't want to import rdflib

the reason it doesn't fail in CI is cause we do a pip install -r requirements.txt in travis... maybe we shouldn't

Choose a reason for hiding this comment

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

I thought I remembered it being bad for some reason - but figure I'll let travis decide :)

now six is used throughout.

Choose a reason for hiding this comment

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

bugger, this wasn't mean to go in this commit.

replaced with doctest-ignore-unicode noseplugin

tests pass, i went over the last couple of commits and quickly skimmed the whole (as good as possible with 156 changed files)... in the interest of finishing this ~ 1.5 years process, i'm just going ahead and merging this... other bugs and cleanups can be fixed later

Choose a reason for hiding this comment

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

@joernhees after I went to bed I realise that this should have been in test_requires in setup.py and not here :D

And then we could run the tests with python setup.py nosetests on travis.

But we can fix in master!