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

Fix the Windows support #345

Merged
merged 3 commits into from
Aug 17, 2016
Merged

Fix the Windows support #345

merged 3 commits into from
Aug 17, 2016

Conversation

liZe
Copy link
Member

@liZe liZe commented Aug 16, 2016

I've tried many little things to make WeasyPrint work with both Python 2 and 3, and here's the best solution I've found.

  • As explained in Fix Python 3 compatibility on Windows #132, the problem is that pathname2url doesn't accept bytestrings with Python 3 on Windows. As Python 2 allows bytes and Python 3 allows unicode with all OSes, I've added the encode in the compat module's pathname2url implementation.
  • Contrary to what I've said, there's no need to be afraid of returning unicode strings in path2url: unicode is actually already returned because of the implicit cast due to u'abc' + b'abc' at the end of the function.
  • @SimonSapin's idea to use open instead of urlopen is a good idea, but it's not related to this problem. For the record, I've tried to use open, but URIs for Windows filenames have an extra slash before the volume (something like /C:/path). This leading path is explained by the RFC and it's handled correctly by urlopen, duplicating code to handle it (and other needed features such as guessing mimetypes) in WP may be useless. We can do this later if we want.
  • I've put the FILESYSTEM_ENCODING variable in the compat module: it's not a bad place and avoids a circular import.

Finally, this PR is pretty equivalent to #132, but a bit cleaner 😄.

It's been tested with Python 2.7 and 3.5, on Windows 10 and Linux. Some tests launched with py.test even work on Windows (but some tests make Python crash, fun fun fun).

@SAPikachu
Copy link
Contributor

Thanks! Just tried and it seems to work fine here.

@liZe
Copy link
Member Author

liZe commented Aug 17, 2016

Let's go!

@liZe liZe merged commit 3b2cef6 into master Aug 17, 2016
@liZe liZe deleted the filenames branch August 17, 2016 14:50
@liZe liZe modified the milestone: v0.31 Aug 17, 2016
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.

2 participants