Fix importlib.metadata.files workaround #702

kaechele · 2023-05-23T21:27:27Z

kaechele commented

2023-05-23 21:27:27 +00:00

Some distributions, such as Fedora, do not install the RECORDS file as
part of a package's dist-info. As a result importlib.metadata.files will
return None.

Use the workaround for these cases as well.

Signed-off-by: Felix Kaechele felix@kaechele.ca

Some distributions, such as Fedora, do not install the RECORDS file as part of a package's dist-info. As a result importlib.metadata.files will return None. Use the workaround for these cases as well. Signed-off-by: Felix Kaechele <felix@kaechele.ca>

witten reviewed 2023-05-23 21:56:32 +00:00

witten left a comment

Thanks for taking the time to fix this!

A couple of thoughts:

Given the complexity/unreliability of the importlib_metadata approach, what do you think of ditching that whole code path and doing the os.path.join(os.path.dirname(borgmatic.config.__file__), 'schema.yaml') thing unconditionally? Does that seem like it'd work in all cases?
With these changes, I'd appreciate if you also updated / added to the the existing unit tests for this function in tests/unit/config/test_validate.py.

Thanks for taking the time to fix this! A couple of thoughts: * Given the complexity/unreliability of the `importlib_metadata` approach, what do you think of ditching that whole code path and doing the `os.path.join(os.path.dirname(borgmatic.config.__file__), 'schema.yaml')` thing unconditionally? Does that seem like it'd work in all cases? * With these changes, I'd appreciate if you also updated / added to the the existing unit tests for this function in `tests/unit/config/test_validate.py`.

kaechele commented

2023-05-25 00:39:45 +00:00

Sure thing!

I think it makes sense to just determine the location of the schema based on the config.__file__. As long as the Python packaging tooling installs it alongside that file (and so far it does) we should be good.

I'll take another look at it and amend the pull request accordingly.

Sure thing! I think it makes sense to just determine the location of the schema based on the `config.__file__`. As long as the Python packaging tooling installs it alongside that file (and so far it does) we should be good. I'll take another look at it and amend the pull request accordingly.

👍 1

witten referenced this pull request

2023-05-29 22:08:45 +00:00

borgmatic 1.7.13 fails to load config (?) #703

witten commented

2023-06-01 16:36:47 +00:00

I was hoping to get this fix into the next release. If you won't have a chance to work on this soon, just let me know and I can pick it up from here. Thank you!

kaechele force-pushed fix-importlib-files from b316ccbdc5 to 5c6a61fd14

2023-06-03 01:43:07 +00:00

Compare

kaechele force-pushed fix-importlib-files from 5c6a61fd14 to 8d5cc53813

2023-06-03 01:44:46 +00:00

Compare

kaechele commented

2023-06-03 01:49:02 +00:00

Alright. I've updated my branch. Somehow managed to get it out of sync, hence the force push. Sorry about that. I usually prefer working on branches iteratively and have them squashed into one commit that gets merged at the end.

One thing I was wondering is whether we should validate that a file exists at that path so that we can raise FileNotFoundError from that function if required (which seems to have been the original behaviour in the first place).

Alright. I've updated my branch. Somehow managed to get it out of sync, hence the force push. Sorry about that. I usually prefer working on branches iteratively and have them squashed into one commit that gets merged at the end. One thing I was wondering is whether we should validate that a file exists at that path so that we can raise FileNotFoundError from that function if required (which seems to have been the original behaviour in the first place).

witten commented

2023-06-03 06:28:19 +00:00

Alright. I've updated my branch. Somehow managed to get it out of sync, hence the force push. Sorry about that. I usually prefer working on branches iteratively and have them squashed into one commit that gets merged at the end.

No worries! (I'm generally pretty loosey-goosey about my own commits on this project.)

One thing I was wondering is whether we should validate that a file exists at that path so that we can raise FileNotFoundError from that function if required (which seems to have been the original behaviour in the first place).

Yes, that seems reasonable. Thank you!

> Alright. I've updated my branch. Somehow managed to get it out of sync, hence the force push. Sorry about that. I usually prefer working on branches iteratively and have them squashed into one commit that gets merged at the end. No worries! (I'm generally pretty loosey-goosey about my own commits on this project.) > One thing I was wondering is whether we should validate that a file exists at that path so that we can raise FileNotFoundError from that function if required (which seems to have been the original behaviour in the first place). Yes, that seems reasonable. Thank you!

witten requested changes 2023-06-04 02:47:15 +00:00

witten left a comment

Thanks for doing another round on this! I left some annoying, nit-picky comments on some of the particulars in this PR. If at any point you get tired of the back-and-forth, please let me know and I'd be happy to finish this up.

borgmatic/config/validate.py Outdated

						
				@@ -31,1 +17,3 @@

				        return os.path.join(os.path.dirname(borgmatic.config.__file__), 'schema.yaml')

				    schema_path = os.path.join(os.path.dirname(borgmatic.config.__file__), 'schema.yaml')

				    if os.path.exists(schema_path) and os.path.isfile(schema_path):

witten commented

2023-06-04 02:39:25 +00:00

Turns out you don't need exists() if you've also got isfile()!

Turns out you don't need `exists()` if you've also got `isfile()`!

kaechele marked this conversation as resolved

borgmatic/config/validate.py Outdated

						
				@@ -32,0 +19,4 @@

				    if os.path.exists(schema_path) and os.path.isfile(schema_path):

				        return schema_path

				    raise FileNotFoundError

witten commented

2023-06-04 02:41:28 +00:00

Just raising the exception type like this means that it won't include the filename in the exception, and therefore any error messages won't tell the user what file wasn't found. One way around that is to set the message manually when you raise the error type. But an even easier way might be to actually open() the file here. Then you won't need the exists()/isfile() checks and it'll conveniently raise a FileNotFoundError populated with an appropriate filename message.

Just raising the exception type like this means that it won't include the filename in the exception, and therefore any error messages won't tell the user what file wasn't found. One way around that is to set the message manually when you raise the error type. But an even easier way might be to actually `open()` the file here. Then you won't need the `exists()`/`isfile()` checks *and* it'll conveniently raise a `FileNotFoundError` populated with an appropriate filename message.

kaechele commented

2023-06-04 04:56:34 +00:00

Went with the open() approach.

Went with the `open()` approach.

tests/unit/config/test_validate.py Outdated

						
				@@ -23,3 +8,1 @@

				    )

				    assert module.schema_filename().endswith('/borgmatic/config/schema.yaml')

				    module.schema_filename().endswith('/borgmatic/config/schema.yaml')

witten commented

2023-06-04 02:44:55 +00:00

Now that the unit under test is calling exists()/isfile() (or open() as suggested above), those functions need to be mocked out here IMO. Otherwise, you run the risk of the environment in which the tests are running impacting the tests. (I realize schema.yaml should always be present.)

Additionally, now that you've got an if in your unit under test, ideally that code path would be tested as well. Which you could do with a separate test function and judicious use of mocks.

Now that the unit under test is calling `exists()`/`isfile()` (or `open()` as suggested above), those functions need to be mocked out here IMO. Otherwise, you run the risk of the environment in which the tests are running impacting the tests. (I realize `schema.yaml` *should* always be present.) Additionally, now that you've got an `if` in your unit under test, ideally that code path would be tested as well. Which you could do with a separate test function and judicious use of mocks.

kaechele commented

2023-06-04 04:57:16 +00:00

I've updated the tests and am getting 100% coverage.

kaechele force-pushed fix-importlib-files from deaa6a9c6f to c61d63b235

2023-06-04 04:55:51 +00:00

Compare

kaechele commented

2023-06-04 04:58:09 +00:00

I actually appreciate nitpicky comments about my PRs as an opportunity to learn. Thanks for taking the time.

witten approved these changes 2023-06-04 05:02:40 +00:00

witten left a comment

Awesome, thank you for making these changes!

tests/unit/config/test_validate.py

						
				@@ -13,3 +14,1 @@

				        flexmock(match=lambda path: False, locate=lambda: None),

				    )

				    flexmock(os.path).should_receive('dirname').and_return("/var/borgmatic/config")