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

Swap unipath with pathlib #356

Closed
wants to merge 1 commit into from
Closed

Conversation

relekang
Copy link
Contributor

This is related to issue #343. Unipath has broken python 3 support.

@jezdez
Copy link
Contributor

jezdez commented Jan 23, 2015

I'm pretty sure there are more of those hidden in the docs views

@relekang
Copy link
Contributor Author

Thanks, I'll fix the rest later today

@relekang
Copy link
Contributor Author

I updated it now. The docs site and ./manage.py update_docs works locally.


# Utilities

# The full path to the repository root.
BASE = Path(__file__).absolute().ancestor(2)
BASE = Path(__file__).resolve().parents[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think using Path(__file__).resolve().parent would be nicer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it needs to be Path(__file__).resolve().parent.parent, since Path.parent is the same as Path.parents[0]. Do you still think it is nicer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yeah. That's still nicer I think, since it's clearer.

@jezdez
Copy link
Contributor

jezdez commented Jan 25, 2015

This is awesome work @relekang, thank you! I've commented a bit above with general directions we should take here parent instead of parents[0] and using joinpath() instead of the operator overloading (via /). It's simply easier to maintain that way and brings less surprises to contributors.

@jezdez
Copy link
Contributor

jezdez commented Jan 25, 2015

One thing that made me always wonder about pathlib is what makes Unipath not require the str() calls around settings for example that pathlib doesn't have?

@relekang
Copy link
Contributor Author

One thing that made me always wonder about pathlib is what makes Unipath not require the str() calls around settings for example that pathlib doesn't have?

It's probably because AbstractPath in unipath inherits from str.

@relekang
Copy link
Contributor Author

I have removed the operator usage and updated it to use parent instead of parents

@timgraham
Copy link
Member

Is this branch meant to be compatible with both Python 2 and 3?

@relekang relekang mentioned this pull request Jan 28, 2015
@relekang
Copy link
Contributor Author

Yes, that was the thought :)

This is related to issue django#343. Unipath has broken python 3 support.
@timgraham
Copy link
Member

I'm a bit confused on the added str stuff cause that's "bytes" on Python 2 and "unicode" on Python 3, right?

@relekang
Copy link
Contributor Author

You are right, I did write it for python 3 and tested if it works on python 2 as well and did not think it through. It works since all the places it is used takes unicode or bytes on python 2. I can close this and make it a part of a python 3 pull-request or should I use django.utils.encoding.smart_text instead?

@timgraham
Copy link
Member

Let's make a Python 3 pull request. I have time to do some manual testing tomorrow as well as update the server deployment, so let's see if we can get this finished off in the next couple days.

@relekang relekang closed this Jan 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants