borgmatic-collective/borgmatic
borgmatic-collective
/
borgmatic
15
57
Fork 40
Code Issues 54 Pull Requests 1 Actions Packages Releases 121 Activity

Expand source directories when checking for existence. #683

Merged
witten merged 1 commits from holocronweaver/borgmatic:master into main 2023-04-21 06:06:00 +00:00
Conversation 5 Commits 1 Files Changed 2 +16 -1
holocronweaver commented 2023-04-20 01:56:58 +00:00
Contributor
Copy Link

Should resolve #682.

This is my first pull request to borgmatic, so please let me know if I'm doing it wrong. πŸ˜„

Tested on my own config files that use a mix of glob and tilde source directories.

Verified unit test fails with original existence check, and passes with updated check.

Should resolve #682. This is my first pull request to borgmatic, so please let me know if I'm doing it wrong. πŸ˜„ Tested on my own config files that use a mix of glob and tilde source directories. Verified unit test fails with original existence check, and passes with updated check.
witten requested changes 2023-04-20 04:17:25 +00:00
borgmatic/borg/create.py Outdated
@ -315,3 +315,3 @@
source_directory
for source_directory in source_directories
if not os.path.exists(source_directory)
if not all(map(os.path.exists, expand_directory(source_directory)))
witten commented 2023-04-20 04:09:51 +00:00
Owner
Copy Link

Nice! This looks like it'd do it. FYI I generally don't use map() in this code base when a list/tuple comprehension will work, just because I find map() a tiny bit less readable even if it's more succinct. But it's certainly not a hard-and-fast rule, so feel free to keep this as-is.

Nice! This looks like it'd do it. FYI I generally don't use `map()` in this code base when a list/tuple comprehension will work, just because I find `map()` a tiny bit less readable even if it's more succinct. But it's certainly not a hard-and-fast rule, so feel free to keep this as-is.
holocronweaver commented 2023-04-21 02:09:10 +00:00
Author
Contributor
Copy Link

Fair enough! Will switch to list comprehension to better fit existing code style. Agree it is more readable, which is the common case so makes sense to optimize style for it.

Fair enough! Will switch to list comprehension to better fit existing code style. Agree it is more readable, which is the common case so makes sense to optimize style for it.
holocronweaver marked this conversation as resolved
tests/unit/borg/test_create.py Outdated
@ -2567,1 +2567,4 @@
)
def test_create_archive_with_glob_and_tilde_source_directories_and_source_directories_must_exist():
witten commented 2023-04-20 04:15:31 +00:00
Owner
Copy Link

The bad news: You've written an integration test! Specifically, integrating the two units create_archive and check_all_source_directories_exist along with testing some of their respective functionality. (Some might argue that they are part of the same unit as one of them is "just" a utility function, but personally I've never subscribed to that view.)

The good news: I think the only test you need to write is much smaller and simpler than this one. IMO all you need to unit test is check_all_source_directories_exist(). I don't see an existing test for that(!), so don't feel obliged to test all the cases; just a test for the code path you added would be fine.

Let me know if you have any questions. And thank you for taking this on!

The bad news: You've written an integration test! Specifically, integrating the two units `create_archive` and `check_all_source_directories_exist` along with testing some of their respective functionality. (Some might argue that they are part of the same unit as one of them is "just" a utility function, but personally I've never subscribed to that view.) The good news: I think the only test you need to write is much smaller and simpler than this one. IMO all you need to unit test is `check_all_source_directories_exist()`. I don't see an existing test for that(!), so don't feel obliged to test all the cases; just a test for the code path you added would be fine. Let me know if you have any questions. And thank you for taking this on!
holocronweaver commented 2023-04-21 02:14:16 +00:00
Author
Contributor
Copy Link

Will switch from integ to unit test, which will definitely be simpler and more pointed. I was trying to copy the style of the only test I saw using source_directories_must_exist, without reading enough other tests to notice not all were integ tests.

Will switch from integ to unit test, which will definitely be simpler and more pointed. I was trying to copy the style of the only test I saw using `source_directories_must_exist`, without reading enough other tests to notice not all were integ tests.
holocronweaver marked this conversation as resolved
witten commented 2023-04-20 04:25:19 +00:00
Owner
Copy Link

FYI, related to this, I just fixed a create_archive() unit test to properly mock out check_all_source_directories_exist(), which makes the missing test coverage more evident: 8bb7631f50

FYI, related to this, I just fixed a `create_archive()` unit test to properly mock out `check_all_source_directories_exist()`, which makes the missing test coverage more evident: https://projects.torsion.org/borgmatic-collective/borgmatic/commit/8bb7631f50998d9b0cd796f3c098162011bc1bc7
πŸ‘€ 1
holocronweaver force-pushed master from 56ff38f09f to 2741160a45 2023-04-21 02:01:01 +00:00 Compare
holocronweaver force-pushed master from 2741160a45 to 31e5d3c0b1 2023-04-21 02:15:24 +00:00 Compare
holocronweaver commented 2023-04-21 02:22:43 +00:00
Author
Contributor
Copy Link

Updated PR based on feedback, things are looking cleaner now thanks to your suggestions. πŸ˜„

Verified again that test fails with original check_all_source_directories_exist function and passes after update.

Updated PR based on feedback, things are looking cleaner now thanks to your suggestions. πŸ˜„ Verified again that test fails with original `check_all_source_directories_exist` function and passes after update.
holocronweaver requested review from witten 2023-04-21 02:22:50 +00:00
holocronweaver force-pushed master from 31e5d3c0b1 to 1fdbfec04d 2023-04-21 02:27:22 +00:00 Compare
witten commented 2023-04-21 05:05:48 +00:00
Owner
Copy Link

This looks great to me, thank you! Feel free to remove the WIP tag whenever you're ready and update the branch from main, if you would please. Then I can merge it!

This looks great to me, thank you! Feel free to remove the WIP tag whenever you're ready and update the branch from main, if you would please. Then I can merge it!
πŸ‘ 1
holocronweaver changed title from WIP: Expand source directories when checking for existence. to Expand source directories when checking for existence. 2023-04-21 05:51:17 +00:00
holocronweaver force-pushed master from 1fdbfec04d to a14870ce48 2023-04-21 05:52:05 +00:00 Compare
holocronweaver commented 2023-04-21 05:53:51 +00:00
Author
Contributor
Copy Link

Rebased and removed WIP tag, should be good merge. Thanks for the speedy review!

Rebased and removed WIP tag, should be good merge. Thanks for the speedy review!
witten merged commit 5829196b70 into main 2023-04-21 06:06:00 +00:00
witten commented 2023-04-21 06:06:25 +00:00
Owner
Copy Link

Awesome, thank you!

Awesome, thank you!
Sign in to join this conversation.
Reviewers
No reviewers
witten
Labels
Clear labels   
bug

   
data loss

   
design finalized

   
good first issue

   
new feature area

   
question / support

   
security

   
waiting for response
No Label
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#683
No description provided.
Delete Branch "holocronweaver/borgmatic:master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?