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 some unicode bugs on Python 2. #63

Merged
merged 28 commits into from Jul 5, 2021
Merged

Conversation

mcmtroffaes
Copy link
Collaborator

To reproduce some issues reported by @ppentchev.

@mcmtroffaes
Copy link
Collaborator Author

@ppentchev I think your patch only makes sense if we also do this:

9ad08c7

What test case triggered your original issue?

@mcmtroffaes mcmtroffaes changed the title Add some unicode tests. Fix some unicode bugs on Python 2. Jul 5, 2021
@ppentchev
Copy link
Contributor

Hi,

Sorry, I pushed one additional commit into my branch before I saw that you closed #62 - take a look at ppentchev@7899f60 to see my test case. Of course, it is entirely possible that it is not complete or comprehensive, and it is absolutely, 100% possible that I do not yet fully understand the internal workings of pathlib2 - I completely defer to you on that.

Still, the test_unicode.py file that I added in the above-mentioned commit seems to work without your _py2_fsencode() changes, but, again, it is entirely possible that I've missed something... come to think of it, yes, it's kind of obvious that I did not really test with characters that are outside the US-ASCII range! Aaaaaaargh! :) Thanks A LOT for actually taking the time to come up with a real test, not my lame excuse for one! :)

G'luck,
Peter

@ppentchev
Copy link
Contributor

To clarify my comment above: it's true that I did not really test with characters outside the US-ASCII range. The reasons for that are twofold:

  • this issue came to my attention when I was trying to make a somewhat-comprehensive test case for mypy's typeshed stubs for pathlib2, and all I tried to do was pass Unicode strings as parameters to the pathlib2 methods so that mypy would complain and I would fix the stubs that it complained about
  • I had the insanely unwise idea to try and finish this after midnight on a Sunday night... so, yeah, my thinking may not have been entirely straight at some point

So..... yeah, once again, thanks a lot for 1. agreeing to reopen pathlib2, 2. taking the time to look into my code, and 3. actually going in and making a better fix yourself before I could get around to providing you with my test case!

G'luck,
Peter

@mcmtroffaes
Copy link
Collaborator Author

Lovely, thanks for sharing! It's now part of the test suite. I'll run a few more tests and then cut a release on pypi.

@ppentchev
Copy link
Contributor

BTW, the mypy --py2 problems may be fixed by providing your own *.pyi file, e.g. the one that was accepted into the typeshed project earlier today exactly with the Unicode fixes: see python/typeshed#5727
Of course, if you decide that you do not want to make such a change - start carrying type definitions - right now, that's all right, too.

@ppentchev
Copy link
Contributor

Ah... and I just realized that carrying your own *.pyi file would help for the public API, but not for the internal functions - for those, you may want to add type hints by hand..... which might very well not be something you want to do right now.

@mcmtroffaes
Copy link
Collaborator Author

Sure, for now, I'm happy for typeshed to carry these. And many thanks for adding type checking to the library! ❤️

@mcmtroffaes mcmtroffaes merged commit c9ecd0b into develop Jul 5, 2021
@mcmtroffaes mcmtroffaes deleted the feature/with_name_unicode branch July 5, 2021 11:48
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.

None yet

2 participants