Skip to content

Catch exception if the distribution is not found#472

Closed
s4heid wants to merge 3 commits into
pyeve:masterfrom
s4heid:handle-distribution-error
Closed

Catch exception if the distribution is not found#472
s4heid wants to merge 3 commits into
pyeve:masterfrom
s4heid:handle-distribution-error

Conversation

@s4heid
Copy link
Copy Markdown
Contributor

@s4heid s4heid commented Mar 12, 2019

When the distribution of Cerberus cannot be found, catch the exception
instead of raising an error and exiting the program. This allows to
consume the package without installing it, e.g. as a git submodule. In
the exception case the version string is set to unknown.

Fixes #471

Copy link
Copy Markdown
Member

@funkyfuture funkyfuture left a comment

Choose a reason for hiding this comment

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

looks mostly good. can you please also update the AUTHORS and CHANGES.rst document?

are you familiar with pytest's monkeypatch fixture? that would be the only feasible way to test your addition. as the problem is small here, it's actually a good opportunity to get a grip on it.

Comment thread cerberus/__init__.py Outdated
@funkyfuture
Copy link
Copy Markdown
Member

sorry, i forgot that the code to be tested it executed at module evaluation. the unit-testable approach would be to put the questionable in a function and call that from the module body. i'd say do as you like.

s4heid added a commit to s4heid/cerberus that referenced this pull request Mar 14, 2019
@s4heid
Copy link
Copy Markdown
Contributor Author

s4heid commented Mar 14, 2019

Agreed; I also had this in mind at the beginning. Another solution would be to mock the pkg_resources.get_distribution method and patch the builtin import function. @funkyfuture wdyt?

s4heid added 2 commits March 14, 2019 15:42
When the distribution of Cerberus cannot be found, catch the exception
instead of raising an error and exiting the program. This allows to
consume the package without installing it, e.g. as a git submodule. In
the exception case the __version__ string is set to `unknown`.

Fixes pyeve#471
@s4heid s4heid force-pushed the handle-distribution-error branch from 79b6c7c to 77ec23a Compare March 14, 2019 14:46
Copy link
Copy Markdown
Member

@funkyfuture funkyfuture left a comment

Choose a reason for hiding this comment

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

i was quiet distracted during the review, please excuse if i may have confused something on the way.

Comment thread cerberus/tests/conftest.py Outdated
Comment thread cerberus/tests/test_init.py Outdated
Comment thread cerberus/tests/conftest.py Outdated
Comment thread cerberus/tests/test_init.py Outdated
This includes:
* moving the tests from test_init.py into test_assorted.py
* rely on PYTHON_VERSION variable rather than the nested try/except
* monkeypatch inside the test function and get rid of cerberus fixture
@s4heid
Copy link
Copy Markdown
Contributor Author

s4heid commented Mar 15, 2019

Thanks for the review, @funkyfuture. Your suggestions made perfect sense to me.

Copy link
Copy Markdown
Member

@funkyfuture funkyfuture left a comment

Choose a reason for hiding this comment

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

that's perfect.

@funkyfuture
Copy link
Copy Markdown
Member

merged w/ 7896257

@funkyfuture
Copy link
Copy Markdown
Member

thanks a lot for your contribution!

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.

2 participants