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

Add stat times #13

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

Add stat times #13

wants to merge 6 commits into from

Conversation

WeatherGod
Copy link

sftpserver should now manage atime and mtime of the "files", and operating on the filesystem should get you proper atime/mtime modification behavior.

Needed for writing tests for an internal package of my employer that checks the lateness of product deliveries.

I had to mark two old test to skip because they no longer work, but I am not convinced the tests were correct before.

@coveralls
Copy link

coveralls commented Jun 9, 2017

Coverage Status

Coverage decreased (-0.9%) to 97.531% when pulling c8846bd on WeatherGod:add_stat_times into 547b1d3 on ulope:master.

@WeatherGod
Copy link
Author

Travis failure is due to the lack of py26 on the travis image, and the decrease in coverage is mostly due to me marking two tests for skipping because I couldn't figure out if they were right or not.

This PR merges cleanly with #11, but will need one line modified after the merge in interface.VirtualSFTPHandle.write().

@WeatherGod
Copy link
Author

I rebased this PR onto master.

@WeatherGod
Copy link
Author

Interesting that this fails on windows only, but not linux. The failures on windows basically show that the mtimes aren't getting updated.

except TypeError:
return len(str(self.get(path)))
return len(str(self.get(path, atime_change=False)))
Copy link
Author

Choose a reason for hiding this comment

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

Just noticed that this would be wrong in py3k in light of #15 (which doesn't fix this).

@ulope ulope force-pushed the master branch 13 times, most recently from bfaaa6d to 95a3bc8 Compare September 16, 2019 13:02
@WeatherGod
Copy link
Author

Let me know if you'd like me to try rebasing this branch to take into account all of the recent changes. I did it once before, so maybe I can find that branch somewhere...

@ulope
Copy link
Owner

ulope commented Sep 17, 2019

@WeatherGod if you could that would be nice!

WeatherGod and others added 6 commits September 17, 2019 12:58
* added recursive_list() method
* parametrized some existing tests
* marked a broken test to skip
* added tests to ensure times are managed correctly
  before and after file operations
it will trigger a rebuild of the time dictionary. This is needed
for the .serve_content() method of sftpserver.

Also refactor the tests a bit to reduce redundant code.
time-handling features of ContentProvider. Also refactor the
unit tests to reduce redundant code.
@WeatherGod
Copy link
Author

rebased and forced-pushed. Also addressed the comment I made about the additional line of code that needed merging.

Don't forget that my changes also includes marking two existing tests for skipping, as I think they are wrong, but not being the original author, I can't be certain.

@WeatherGod
Copy link
Author

seriously, a simple, small, recursive function is considered "too complex", and for a concept that nearly any programmer should understand (recursive listing of a directory structure...)? Bah!

@ulope
Copy link
Owner

ulope commented Sep 17, 2019

Thanks!

Don't forget that my changes also includes marking two existing tests for skipping, as I think they are wrong, but not being the original author, I can't be certain.

I’ll have a look.

As for the code climate warnings, I agree threat they are a bit over the top. The thresholds seem to be configurable. I’ll see if I can chill it out a bit. But it’s not a mandatory check anyway.

@WeatherGod
Copy link
Author

Weird, this is still showing the same problem on Windows that I noticed before where the mtimes aren't getting updated. This problem doesn't happen on my linux systems. I am not sure why this would only impact windows, unless maybe paramiko is doing something different on windows than on linux? I am not familiar enough with windows to know.

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.

3 participants