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

Allow timespan to be specified with common time units #8626

Merged
merged 9 commits into from
Jan 8, 2025

Conversation

KenKundert
Copy link
Contributor

@KenKundert
Copy link
Contributor Author

This version supports Y/m/d and H/M/S as you specified. Please let me know if you want to support weeks as well. Also, let me know you want make all units other than m and M case insensitive.

@jdchristensen
Copy link
Contributor

I think we should match how borg prune --keep-within works, which allows weeks. It's probably case sensitive, but someone could check the code. If we support minutes and seconds here, we should probably do the same with --keep-within as well.

The --keep-within option takes an argument of the form “<int><char>”, where char is “H”, “d”, “w”, “m”, “y”. For example, --keep-within 2d means to keep all archives that were created within the past 48 hours. “1m” is taken to mean “31d”. 

@ThomasWaldmann
Copy link
Member

Yeah, what @jdchristensen says. Consistency within borg is of course more important than consistency to strftime formats (which most users are not aware of anyway).

So, ymwd lowercase, HMS uppercase.

Copy link
Member

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

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

consistency feedback. thanks for the PR!

docs/usage/general/date-time.rst.inc Outdated Show resolved Hide resolved
src/borg/helpers/parseformat.py Outdated Show resolved Hide resolved
src/borg/helpers/time.py Show resolved Hide resolved
src/borg/helpers/time.py Outdated Show resolved Hide resolved
Ken Kundert added 2 commits January 6, 2025 12:44
This is initial response to comments made on pull request.  Will look at
--keep-within in a bit.
Copy link

codecov bot commented Jan 6, 2025

Codecov Report

Attention: Patch coverage is 96.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 81.83%. Comparing base (1559a1e) to head (ea65cae).
Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
src/borg/helpers/time.py 92.30% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8626      +/-   ##
==========================================
- Coverage   81.83%   81.83%   -0.01%     
==========================================
  Files          74       74              
  Lines       13319    13340      +21     
  Branches     1963     1969       +6     
==========================================
+ Hits        10900    10917      +17     
- Misses       1755     1758       +3     
- Partials      664      665       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ThomasWaldmann
Copy link
Member

BTW, I just saw that you are working in your local master branch. This can often be a problem (e.g. if you need to rebase onto current upstream master for some reason), so try to remember to always checkout a feature/fix branch first (like git checkout -b myfeature).

It looks like we can quickly merge this PR, so no need to change branches here.

@KenKundert
Copy link
Contributor Author

KenKundert commented Jan 7, 2025

I have enhanced 'prune --keep-within' to support seconds and minutes and checked in the changes. I believe I have also made all requested changes.

I cannot run tests locally, there seems to be some issue with the version, the tests all end in a failure complaining about an invalid version number when I run tests locally. So I am just waiting for you to approve the workflows so I can see how many mistakes I made.

Thanks for the suggestion to create and operate on a feature branch. I have limited experience contributing to projects that are not my own.

@ThomasWaldmann
Copy link
Member

@KenKundert Likely you did not fetch the tags. setuptools_scm creates the version number based on git tags.

Copy link
Member

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

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

Some minor stuff I found.

src/borg/helpers/parseformat.py Outdated Show resolved Hide resolved
src/borg/helpers/parseformat.py Outdated Show resolved Hide resolved
src/borg/testsuite/helpers_test.py Outdated Show resolved Hide resolved
src/borg/testsuite/helpers_test.py Outdated Show resolved Hide resolved
Copy link
Member

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

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

found a typo

src/borg/testsuite/helpers_test.py Outdated Show resolved Hide resolved
@KenKundert
Copy link
Contributor Author

KenKundert commented Jan 8, 2025

I finally was able to fetch the tags and the tests now are working locally. So going forward there should be many fewer of my stupid mistakes to review. I apologize for the churn.

I think I am basically done, though I will add in support for quarters if you wish.

Copy link
Member

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

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

LGTM!

@ThomasWaldmann ThomasWaldmann merged commit b9498ca into borgbackup:master Jan 8, 2025
15 of 16 checks passed
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