wagtail-streamfield-migration-toolkit icon indicating copy to clipboard operation
wagtail-streamfield-migration-toolkit copied to clipboard

Comparison of `args` and `kwargs` returned by `deconstruct` may result in poor comparisons

Open jams2 opened this issue 2 years ago • 1 comments

Currently in BaseBlockDefComparer we compare blocks for similarity by deconstructing them and comparing the args and kwargs. This may cause poor comparisons when, for example, positional arguments are passed as keyword arguments, for example:

>>> from wagtail.snippets.blocks import SnippetChooserBlock
>>> a = SnippetChooserBlock("myapp.MyModel")
>>> b = SnippetChooserBlock(target_model="myapp.MyModel")
>>> a.deconstruct()
('wagtail.snippets.blocks.SnippetChooserBlock', ('myapp.MyModel',), {})
>>> b.deconstruct()
('wagtail.snippets.blocks.SnippetChooserBlock', (), {'target_model': 'myapp.MyModel'})

In this case, blocks a and b are evidently "the same", but will not be considered so under the current logic.

Examples

To illustrate the cases we will need to consider to solve this, take the following class as an example. Any new object created gets a copy of the instantiation args and kwargs cached on it, as in the base Block class.

class Foo:
    def __new__(cls, *args, **kwargs):
        obj = super().__new__(cls)
        obj._constructor_args = (args, kwargs)
        return obj

    def __init__(self, bar, baz, *args, qux=None, **kwargs):
        pass

Positional arguments may be passed as keyword arguments:

>>> Foo(bar="bar", baz="baz", qux="qux")._constructor_args
((), {'bar': 'bar', 'baz': 'baz', 'qux': 'qux'})

Keyword arguments may be passed positionally:

>>> Foo("bar", "baz", "qux")._constructor_args
(('bar', 'baz', 'qux'), {})

There may be varargs:

>>> Foo("bar", "baz", "qux", 42)._constructor_args
(('bar', 'baz', 'qux', 42), {})

There may be varkwargs:

>>> Foo("bar", "baz", qux="qux", quux="quux")._constructor_args
(('bar', 'baz'), {'qux': 'qux', 'quux': 'quux'})

There may be an (unholy) combination of varargs and varkwargs

>>> Foo("bar", "baz", "qux", 42, qux="qux", quux="quux")._constructor_args
(('bar', 'baz', 'qux', 42), {'qux': 'qux', 'why': 'quux'})

Suggestions

The inspect module has a number of utilities that may be of use.

inspect.getfullargspec returns a namedtuple with the function's argument spec:

>>> inspect.getfullargspec(Foo.__init__)
FullArgSpec(args=['self', 'bar', 'baz'], varargs='args', varkw='kwargs', defaults=None, kwonlyargs=['qux'], kwonlydefaults={'qux': None}, annotations={})

inspect.bind can create an inspect.BoundArguments object:

>>> sig = inspect.Signature.from_callable(Foo.__init__)
>>> sig
<Signature (self, bar, baz, *args, qux=None, **kwargs)>
>>> foo = Foo("bar", "baz", "qux", 42, qux="qux", quux="quux")
>>> bound_args = sig.bind(foo, *foo._constructor_args[0], **foo._constructor_args[1])
>>> bound_args
<BoundArguments (self=<__main__.Foo object at 0x7fcd4599f880>, bar='bar', baz='baz', args=('qux', 42), qux='qux', kwargs={'quux': 'quux'})>

Which we could potentially use to take care of resolving differences in the way a function can be called:

>>> sig.bind(foo, bar="bar", baz="baz")
<BoundArguments (self=<__main__.Foo object at 0x7fcd4599f880>, bar='bar', baz='baz')>
>>> sig.bind(foo, "bar", "baz")
<BoundArguments (self=<__main__.Foo object at 0x7fcd4599f880>, bar='bar', baz='baz')>
>>> sig.bind(foo, bar="bar", baz="baz") == sig.bind(foo, "bar", "baz")
True

jams2 avatar Nov 11 '22 19:11 jams2

inspect.Signature and BoundArguments have been around since 3.3: https://peps.python.org/pep-0362/

jams2 avatar Nov 11 '22 19:11 jams2