-
Notifications
You must be signed in to change notification settings - Fork 105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BUG: fence pytest import in tomo testutils #982
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment
odl/tomo/util/testutils.py
Outdated
# Make trivial decorator | ||
from odl.util import OptionalArgDecorator | ||
|
||
class ident(OptionalArgDecorator): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we need to overwrite this, shouldn't this be the default implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean the _wrapper
? That would make sense, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the _wrapper
Needs approval |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well minor fix then go
odl/tomo/util/testutils.py
Outdated
|
||
skip_if_no_astra = pytest.mark.skipif("not odl.tomo.ASTRA_AVAILABLE", | ||
reason='ASTRA not available') | ||
class ident(OptionalArgDecorator): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why even do this? Instead do
from odl.util import OptionalArgDecorator as ident
Fix in other places as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha!
94c3d28
to
dc9c119
Compare
Another thing that needs last-minute fix. Import in a new environment without
pytest
installed would fail.