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

Added support for MDTM command in FTPFS #462

Merged
merged 9 commits into from
Jul 3, 2021

Conversation

atollk
Copy link
Member

@atollk atollk commented Mar 20, 2021

Type of changes

  • Other

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

Fixes #456

Added a getmodified function to fs.base.FS which defaults to calling getinfo. In FTPFS, a check for the support of MDTM was added to provide a more efficient command for this specialized request.

Two calls to FS.getinfo(...).modified in fs.copy were replaced by calls to FS.getmodified.

@coveralls
Copy link

coveralls commented Mar 20, 2021

Coverage Status

Coverage decreased (-0.006%) to 95.26% when pulling 4feb242 on atollk:issue_456 into ac8a91a on PyFilesystem:master.

@atollk
Copy link
Member Author

atollk commented Mar 20, 2021

Not sure how I would add tests for this. Are there any tests that run with an actual FTP server? Should I do extensive mocking to simulate a server that supports MDTM?

@althonos althonos added this to the v2.5.0 milestone Apr 1, 2021
@atollk
Copy link
Member Author

atollk commented Apr 7, 2021

I added a test to test_ftpfs which, I think, decently covers this new feature, so I consider the PR ready to be reviewed / merged now.

A new namespace "modified" was introduced to the `getinfo` function to potentially use cheaper commands compared to the general "details" namespace. In particular, `FTPFS` now uses the MDTM command if supported by the server.
atollk added 2 commits April 15, 2021 09:33
A new namespace "modified" was introduced to the `getinfo` function to potentially use cheaper commands compared to the general "details" namespace. In particular, `FTPFS` now uses the MDTM command if supported by the server.
Copy link
Member

@althonos althonos left a comment

Choose a reason for hiding this comment

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

Great work! A few fixes here and there, but the global structure is as intended 😄

fs/ftpfs.py Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
fs/base.py Outdated Show resolved Hide resolved
tests/test_memoryfs.py Outdated Show resolved Hide resolved
tests/test_memoryfs.py Outdated Show resolved Hide resolved
fs/info.py Outdated Show resolved Hide resolved
@atollk atollk requested a review from althonos April 28, 2021 23:24
@atollk
Copy link
Member Author

atollk commented May 26, 2021

Hi. Any update on this?

@althonos
Copy link
Member

@atollk : sorry for the delay, I had a very intense month but I have a bit more time now.

I changed the current implementation a bit because the Info.modified approach had one caveat: it would cause an Info object to be created without the basic namespace, which it should normally always have, so I was a bit worried about the rest of the interface and that's why I didn't merge right away.

Instead, I kept the FS.getmodified method, and simply overloaded it in FTPFS. This means that code using getmodified should be faster now, and since you updated the relevant functions in fs.copy I think this will successfully fix #456 nevertheless.

@althonos
Copy link
Member

@atollk : All clear now. I'll leave you time to object if there is anything to change, ping me when you're done reading and I'll merge.

tests/test_ftpfs.py Outdated Show resolved Hide resolved
@atollk
Copy link
Member Author

atollk commented Jul 2, 2021

Except for that one comment I made it looks good to me 👍

@althonos althonos merged commit 12cd2f4 into PyFilesystem:master Jul 3, 2021
@althonos
Copy link
Member

althonos commented Jul 3, 2021

Perfect, merging then! Sorry for the delay.

althonos added a commit that referenced this pull request Mar 13, 2022
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.

FTPFS should use MDTM
4 participants