Skip to content

Fix a border case for local fs copy from local#504

Merged
tramora merged 1 commit intodevfrom
fix-border-case-local-fs-copy-from-local
Nov 20, 2025
Merged

Fix a border case for local fs copy from local#504
tramora merged 1 commit intodevfrom
fix-border-case-local-fs-copy-from-local

Conversation

@tramora
Copy link
Copy Markdown
Collaborator

@tramora tramora commented Nov 14, 2025

Fix a border case in the LocalFileSystem.copy_from_local where a relative path is given for the target file.

The library would try and fail to create a parent directory named ''


TODO Before Asking for a Review

  • Rebase your branch to the latest version of dev (or main for release PRs)
  • Make sure all CI workflows are green
  • When adding a public feature/fix: Update the Unreleased section of CHANGELOG.md (no date)
  • Self-Review: Review "Files Changed" tab and fix any problems you find
  • API Docs (only if there are changes in docstrings, rst files or samples):
    • Check the docs build without warning: see the log of the API Docs workflow
    • Check that your changes render well in HTML: download the API Docs artifact and open index.html
    • If there are any problems it is faster to iterate by building locally the API Docs

@tramora tramora force-pushed the fix-border-case-local-fs-copy-from-local branch from 1ec4725 to 72461cb Compare November 14, 2025 14:04
@tramora tramora force-pushed the fix-border-case-local-fs-copy-from-local branch from 72461cb to 46c46c3 Compare November 14, 2025 14:33
Copy link
Copy Markdown
Collaborator

@popescu-v popescu-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the comment.

@tramora tramora force-pushed the fix-border-case-local-fs-copy-from-local branch 2 times, most recently from e423b7c to 2b2fda4 Compare November 17, 2025 13:44
@tramora tramora requested a review from popescu-v November 17, 2025 13:46
Copy link
Copy Markdown
Collaborator

@popescu-v popescu-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the remaining comment.

@tramora tramora force-pushed the fix-border-case-local-fs-copy-from-local branch 2 times, most recently from 1900aff to ee63b91 Compare November 18, 2025 23:12
@tramora tramora requested a review from popescu-v November 20, 2025 09:33
Comment on lines +3218 to +3220
with contextlib.suppress(FileNotFoundError):
os.remove(target_file_name1)
os.remove(target_file_name2)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, we could also use pathlib.Path(target_file_name1).unlink(missing_ok=True), which seems simpler and more explicit to me.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improved

Copy link
Copy Markdown
Collaborator

@popescu-v popescu-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but a further simplification can be made in the tests (see comment).

… path is relative

Without this fix the library would try and fail to create a parent directory named ''
@tramora tramora force-pushed the fix-border-case-local-fs-copy-from-local branch from ee63b91 to ac401a2 Compare November 20, 2025 11:32
@tramora tramora merged commit 6aea158 into dev Nov 20, 2025
13 checks passed
@tramora tramora deleted the fix-border-case-local-fs-copy-from-local branch November 20, 2025 11:36
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