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

#556 Fallback to alternate mtime correctly #557

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

james-emerton
Copy link

@james-emerton james-emerton commented Oct 27, 2022

Type of changes

  • Bug fix

Checklist

  • I've run the latest black with default args on new code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
  • I've added tests for new code.
  • I accept that @PyFilesystem/maintainers may be pedantic in the code review.

Description

This addresses an issues where write_zip() assumed that the details namespace was always available. The stat namespace will be checked first, followed by details, and finally the current time will be used if st_mtime is still None.

I'm willing to author a test against this, but I'm only aware of this being an issue with the s3 filesystem (and only when it encounters a directory) and I'm not certain how best to test that here.

#556

@james-emerton james-emerton changed the title #556 Fallback to alternate mtime depending correctly #556 Fallback to alternate mtime correctly Oct 27, 2022
@althonos
Copy link
Member

althonos commented Dec 2, 2022

Hi James, sorry for the delay in answering, my real-life job has been taking a lot of my time away 😓

A test would be very appreciated, as to avoid any regression (in case the code is changed or refactored, it would be better not to have this issue pop up again later). For this you can probably subclass MemoryFS and make a derived version of getinfo that never returns the details namespace.

@althonos althonos added the bug label Dec 2, 2022
CONTRIBUTORS.md Outdated Show resolved Hide resolved
fs/compress.py Show resolved Hide resolved
@james-emerton
Copy link
Author

@althonos I've added a test for the fallback mtime behaviour, and I think this should now be ready for another look.

tests/test_zipfs.py Outdated Show resolved Hide resolved
tests/test_zipfs.py Outdated Show resolved Hide resolved
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