8000 fix: transfer.get,put remote posix path conversion by elainajones · Pull Request #2333 · fabric/fabric · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: transfer.get,put remote posix path conversion #2333

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: transfer.get,put remote posix path conversion #2333

wants to merge 2 commits into from

Conversation

elainajones
Copy link
@elainajones elainajones commented Apr 1, 2025 •
edited
Loading

Fix posix path conversion for remote paths on the get() and put() methods to support conversion of absolute Windows paths. Without this, paths such as C:\\foo\\bar.txt would be wrongly converted to /C:/Users/someuser/C:\\foo\\bar.txt causing a FileNotFoundError.

@elainajones elainajones force-pushed the elainajones-main branch 2 times, most recently from 3e82472 to 13a35c3 Compare April 1, 2025 22:20
@elainajones
Copy link
Author

Oops! Looks like the tests failed. I'll investigate more when I have a free moment but will likely need help understanding the expected behaviour of the mocks.

@elainajones elainajones mentioned this pull request Apr 1, 2025
Open
@elainajones
Copy link
Author

Related to #2335 which I opened to track the specific bug I'm experiencing. The changes I've added resolve this for my needs locally but obviously I don't want to break support elsewhere. Thanks!

@elainajones elainajones force-pushed the elainajones-main branch 3 times, most recently from 6385b58 to 7f7af02 Compare April 2, 2025 22:57
@elainajones
fix: transfer.get,put remote posix path conversion
5a77764 
Fix posix path conversion for remote paths on the get() and put()
methods to support conversion of absolute Windows paths. Without
this, paths such as `C:\\foo\\bar.txt` would be wrongly converted
to `/C:/Users/someuser/C:\\foo\\bar.txt` causing a
FileNotFoundError.
@elainajones elainajones force-pushed the elainajones-main branch 2 times, most recently from 90b4e12 to bf9e55b Compare April 2, 2025 23:54
@elainajones
test: Add return_value for client.sftp.normalize()
43f5d8c 
Adds return values for the sftp MagicMock to fix the mock object being
passed instead.
@elainajones
Copy link
Author

I think I figured this out. Somehow the mock object is being passed so I need to specify a return value for sftp.normalize similar to how we're already doing for sftp.stat. I got the tests down from 28 to 14.

Obviously, after these pass I'll need to write some additional ones to validate the nonstandard path conversion but that might be tricky to do without it basically resulting in dumb tests against posixpath.join().

Maybe I can get a few pointers as I'm basically figuring this out as I go 😆

@elainajones elainajones force-pushed the elainajones-main branch 2 times, most recently from aac38e5 to 43f5d8c Compare April 3, 2025 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bretk1989 bretk1989 bretk1989 approved these changes

Assignees
No one assigned
Labels
Bug Needs investigation Needs tests Nonstandard platforms put()/get()
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@elainajones @bretk1989 @davidjmemmett
0