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 backport fsencode reference #76

Merged
merged 1 commit into from
Apr 3, 2019
Merged

Fix backport fsencode reference #76

merged 1 commit into from
Apr 3, 2019

Conversation

techalchemy
Copy link
Member

Signed-off-by: Dan Ryan dan@danryan.co

- Fixes #74
- add fsencode and fsdecode test

Signed-off-by: Dan Ryan <dan@danryan.co>
@techalchemy
Copy link
Member Author

@jayvdb seems my fsencode implementation is correct based on it not failing the test you mentioned :p

@techalchemy techalchemy merged commit e2f2b25 into master Apr 3, 2019
@jayvdb
Copy link
Contributor

jayvdb commented Apr 4, 2019

That test is the identity test - the most basic check. Not useful for correctness.

The backports.os repo uses hypothesis to have a substantial test suite and this implementation will surely fail on Linux for Python 2.7, which is where fsencode/fsdecode are most needed. It should be quite easy to drop your implementation into the backports.os test suite so you can see it isnt correct.

And the real os module has a very large test suite indirectly because there are fs unicode tests through the test suite for CPython, not just the os module tests. The backport cant copy any of those, as they rely on other parts of os which are not backported, which is why it needs to have extra hypothesis tests.

This implementation is also very strange as it does not appear to use os.fsencode/fsdecode where possible, which means the problems in this implementation for Linux will affect Python 2 & 3 equally, and Python 3 users will be surprised that vistir-based apps have Python 2 like bugs which other Python 3 apps no longer have.

My guess is this will also fail for anyone on Windows for files not on ntfs; e.g. fat32 partitions. https://github.com/henrysher/duplicity/blob/4ac43220ae5f5df55a9ad4079d8ee400830a5d3c/duplicity/globals.py#L319 for more hints on how to improve that, and of course the Microsoft documentation.

@jayvdb
Copy link
Contributor

jayvdb commented Apr 4, 2019

This implementation is also very strange as it does not appear to use os.fsencode/fsdecode where possible, ...

That is referring to the vistir.compat.fs_encode and fs_decode

The new vistir.backports.tempfile does use os.fsencode and os.fsdecode where available, so should have far fewer problems with encoding.

However I guess from the look of it that tempfile-based functionality will now fail with use of Path-like objects ;-)

@techalchemy techalchemy deleted the bugfix/74 branch May 17, 2019 03:29
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