Expand source directories when checking for existence. #683
Loadingβ¦
x
Reference in New Issue
Block a user
No description provided.
Delete Branch ":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?
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.
@ -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)))
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 findmap()
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.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.
@ -2567,1 +2567,4 @@
)
def test_create_archive_with_glob_and_tilde_source_directories_and_source_directories_must_exist():
The bad news: You've written an integration test! Specifically, integrating the two units
create_archive
andcheck_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!
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.FYI, related to this, I just fixed a
create_archive()
unit test to properly mock outcheck_all_source_directories_exist()
, which makes the missing test coverage more evident:8bb7631f50
56ff38f09f
to2741160a45
2741160a45
to31e5d3c0b1
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.31e5d3c0b1
to1fdbfec04d
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!
WIP: Expand source directories when checking for existence.to Expand source directories when checking for existence.1fdbfec04d
toa14870ce48
Rebased and removed WIP tag, should be good merge. Thanks for the speedy review!
Awesome, thank you!