feat: Support passing arbitrary arguments/context to custom extensions (Issue #700) #814
Conversation
atharva-2001
changed the title
atharva-2001
force-pushed
the
allow_kwargs_extensions
branch
from
26d3347
to
343ae08
Compare
|
@noahnu I'm changing the classes which make up the |
src/syrupy/assertion.py
Outdated
| @@ -264,6 +264,7 @@ def __call__( | |||
| extension_class: Optional[Type["AbstractSyrupyExtension"]] = None, | |||
| matcher: Optional["PropertyMatcher"] = None, | |||
| name: Optional["SnapshotIndex"] = None, | |||
| **kwargs: Any, | |||
There was a problem hiding this comment.
I'm a bit worried about name collisions by just forwarding arbitrary arguments, vs. something like:
assert "hi" == snapshot(extra={...})
I know it doesn't read as nicely, but you can always add syntactic sugar via a fixture:
@pytest.fixture
def snapshot(named_arg: str):
return snapshot.with_defaults(extra={ "named_arg": named_arg })
def test_case(snapshot):
assert "hi" == snapshot(named_arg="hi")
There was a problem hiding this comment.
I added the argument to both __call__ and with_defaults method. I added it to the __call__ argument because I wanted it to work with cases when I want to change the snapshot behaviour but don't want it to persist.
src/syrupy/assertion.py
Outdated
| @@ -280,6 +285,8 @@ def __call__( | |||
| self.__with_prop("_custom_index", name) | |||
| if diff is not None: | |||
| self.__with_prop("_snapshot_diff", diff) | |||
| if extra_args: | |||
| self._extra_args = extra_args | |||
There was a problem hiding this comment.
Use self.__with_prop. It does seam auto-cleanup
src/syrupy/assertion.py
Outdated
| matcher_options = None | ||
| for key,value in self._extra_args.items(): | ||
| if key == "matcher_options": | ||
| matcher_options = value |
There was a problem hiding this comment.
This enforces a certain schema in the extra_args object which if we want to support arbitrary args we shouldn't do. Instead, we should propagate the "extra_args" to all relevant methods.
There was a problem hiding this comment.
Removed this. I also forwaded extra_args to different functions. Could you please tell me what you think about this?
|
We have built-in linters in the project via pyinvoke. You can run |
|
Changing the signatures of all these functions will break any existing extensions in the wild, unless they were already using kwargs. Even then, I spent a couple hours trying to pass the context (or extra_args as you called it) to all relevant functions and the PR grew a bit out of control... I think we need to move back to instance-based extensions (which was something we moved away from in syrupy v4). I've put up #816 to add "context", although context is only available in instance extension methods and there aren't many of those. Once that change is in, we'll have to start migrating from classmethods to instance methods. May need to do that in a syrupy v5 update since it'll be a breaking change to the API. Going back to instance methods also plays nicely with some of the discussion in #754 |
Description
Related to #687 and #700.
Allowing custom
**kwargsto custom extension classes.e.g. This PR refactors the X submodule, applying Y's algorithm to improve Z by K percent.
Related Issues
Checklist
Additional Comments
No additional comments.