-
Notifications
You must be signed in to change notification settings - Fork 59
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
Special handling of bytes_repr for mock filesets #743
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #743 +/- ##
===========================================
+ Coverage 15.14% 84.23% +69.09%
===========================================
Files 53 26 -27
Lines 15693 5137 -10556
Branches 2788 1450 -1338
===========================================
+ Hits 2376 4327 +1951
+ Misses 12988 804 -12184
+ Partials 329 6 -323
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
# Need to disable the mtime cache key for mocked filesets. Used in doctests | ||
@register_serializer(MockMixin) | ||
def bytes_repr_mock_fileset( |
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.
I don't like putting test code in main modules. What about adding pydra.testing.fixtures
and including:
@pytest.fixture(scope='session')
def register_mock_mixin_serializer():
@register_serializer(MockMixin)
...
Then in conftest.py
:
from pydra.testing.fixtures import register_mock_mixin_serializer
This would allow downstream tools that need this feature to also put this import in their conftest.py
.
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.
I agree with the sentiment in general, but in this case I reckon that might be creating a gotcha that would take a bit of tracking down how to solve for little practical benefit, i.e if someone creates a new task package and wants to write a doctest using NiftiGz.mock()
, then they would need to know that they also have to add this fixture
Types of changes
Summary
Implements special bytes_repr function for mocked filesets in(i.e. produced by Nifti1.mock("/path/to/mock")) as the fileset one yields the file mtimes in the cache key, which obviously don't exist for mocked files.
Checklist