-
Notifications
You must be signed in to change notification settings - Fork 8
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 --max-snapshots option #35
Conversation
@Lightjohn Thanks for your pull request, it works! I've done a few small fixes to it on this branch:
There's a couple more things that I'd like to fix before merging this. I'll add comments to them inline in this pull request. Could you pull this branch into your branch, make these fixes, then push to your original branch? I'd also like to add tests for the new |
@@ -378,6 +394,13 @@ def snapshot(source, dest, debug=False, min_snapshots=3, extra_args=None): | |||
""" | |||
date = _datetime() | |||
user, host, snapshots_root = _parse_path(dest) | |||
if max_snapshots > -1 and max_snapshots - min_snapshots < 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could ...and max_snapshots - min_snapshots < 0
just be and max_snapshots < min_snapshots
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about that but I think both ways are good, I can't see counter example at this point.
OK for me as it's cross version (that what not the case for sys.maxint). I add it and made some tests and it should be good. |
Hey John, I got really busy, I promise I'll get back to you on this when I can |
No problem, take your times. I have the code and it's working so no Le 23/02/2016 12:46, Sean Hammond a écrit :
|
No description provided.