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 include/exclude filters #1

Merged
merged 2 commits into from
Jul 19, 2015
Merged

added include/exclude filters #1

merged 2 commits into from
Jul 19, 2015

Conversation

thibault-ketterer
Copy link

Hi,
I added include and exclude filters, hope you find it usefull

@xolox xolox merged commit a1ceb67 into xolox:master Jul 19, 2015
xolox added a commit that referenced this pull request Jul 19, 2015
I made significant changes while merging this (e.g. the short option for
the include list and the use of shell patterns using the fnmatch module)
and I added tests to verify the behavior of the include/exclude logic.
@xolox
Copy link
Owner

xolox commented Jul 19, 2015

Hi and thanks for the pull request! I just merged this and published the result as rotate-backups 1.1. I made significant changes while merging this but I hope you'll agree that they're improvements:

  • You used substring matching while I decided to go with shell pattern matching (using Python's fnmatch module) because it allows for prefix, substring and suffix matching using the * wildcard. In general the use of fnmatch pattern elements allows for more flexible include/exclude matching (IMHO).
  • I rewrote the inner loops to use Python's any() function and generator expressions because this is nicer to read (IMHO).
  • You used -c as the short option for the include list (given that -i was already used for ionice) but I decided to use -I instead. I guess this is just a matter of taste :-).
  • I added a test suite earlier today and decided to write tests for the include/exclude logic as well while merging this.

@thibault-ketterer
Copy link
Author

yup, these are improvements 👍 (tkdbi was also me, github is messing with my old account since I moved the ssh key from one to another)

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