borgmatic-collective/borgmatic
borgmatic-collective
/
borgmatic
14
58
Fork 41
Code Issues 55 Pull Requests 2 Actions Packages Releases 122 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
2 changed files with 16 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
borgmatic/borg/create.py
View File

@ -314,7 +314,7 @@ def check_all_source_directories_exist(source_directories):
missing_directories = [
source_directory
for source_directory in source_directories
if not os.path.exists(source_directory)
if not all([os.path.exists(directory) for directory in expand_directory(source_directory)])
holocronweaver marked this conversation as resolved Outdated
witten commented 2023-04-20 04:09:51 +00:00
Outdated
Review
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
Outdated
Review
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.
]
if missing_directories:
raise ValueError(f"Source directories do not exist: {', '.join(missing_directories)}")

15
tests/unit/borg/test_create.py
View File

@ -2565,3 +2565,18 @@ def test_create_archive_with_non_existent_directory_and_source_directories_must_
storage_config={},
local_borg_version='1.2.3',
)
def test_check_all_source_directories_exist_with_glob_and_tilde_directories():
holocronweaver marked this conversation as resolved Outdated
witten commented 2023-04-20 04:15:31 +00:00
Outdated
Review
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
Outdated
Review
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.
flexmock(module).should_receive('expand_directory').with_args('foo*').and_return(
('foo', 'food')
)
flexmock(module).should_receive('expand_directory').with_args('~/bar').and_return(
('/root/bar',)
)
flexmock(module.os.path).should_receive('exists').and_return(False)
flexmock(module.os.path).should_receive('exists').with_args('foo').and_return(True)
flexmock(module.os.path).should_receive('exists').with_args('food').and_return(True)
flexmock(module.os.path).should_receive('exists').with_args('/root/bar').and_return(True)
module.check_all_source_directories_exist(['foo*', '~/bar'])