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

Unicode file packager #4102

Merged
merged 2 commits into from
Feb 16, 2016
Merged

Unicode file packager #4102

merged 2 commits into from
Feb 16, 2016

Conversation

kripken
Copy link
Member

@kripken kripken commented Feb 14, 2016

Adds a test for #4100.

Passes on linux, since it looks like the error reported in that issue happens in windows-specific code. The code checks for the 'hidden' attribute. @juj, can we just ignore unicode paths in that method perhaps?

@kripken
Copy link
Member Author

kripken commented Feb 14, 2016

Also added a commit that would ignore errors in that method, which seems ok since it just checks for a path being hidden - if we can't check, we might as well continue normally? Or is there a better fix, @juj?

@TannerRogalsky
Copy link
Contributor

Thanks for the quick response, @kripken! My inclination to fix that line was to call decode('unicode-escape') on filepath before passing it to unicode. Seemed to resolve that issue.

The reason I didn't submit a PR with that, though, was that another error occurred after that when generating the json metadata (on this line). Could we also pass a --js-output=test.data.js arg to the file packager in the test and then make that pass?

@juj
Copy link
Collaborator

juj commented Feb 15, 2016

Looks good to me. Tested to pass on Windows as well.

@kripken
Copy link
Member Author

kripken commented Feb 16, 2016

@TannerRogalsky why is that extra flag needed? It just makes it write to a file, not to stdout, and both cases should hit the line you mention with metadata?

@TannerRogalsky
Copy link
Contributor

@kripken You're right. The test passes for me on Windows as well but I still get errors when trying to run the script from various CLIs. This is good for me, though. If someone wants to figure out the precise nature of the issue surrounding unicode encoding of file names in Python 2 in Windows, I will direct them to this starting point.

Thank you!

@kripken
Copy link
Member Author

kripken commented Feb 16, 2016

A possible cause for the failure you still see, @TannerRogalsky, is if not just the dir name but also a filename contains unicode? Worth testing that on windows to see what happens. (On linux it already works here.)

@kripken kripken deleted the unicode_file_packager branch February 16, 2016 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants