Skip to content

FormMixin is incompatible with SuccessMessageMixin #79

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

Closed
PeterJCLaw opened this issue May 15, 2019 · 4 comments · Fixed by #84
Closed

FormMixin is incompatible with SuccessMessageMixin #79

PeterJCLaw opened this issue May 15, 2019 · 4 comments · Fixed by #84

Comments

@PeterJCLaw
Copy link
Contributor

PeterJCLaw commented May 15, 2019

From what I can tell, the Django sources for these two are fine to be mixed together, however the type annotations seem to disagree:

from django.views.generic.edit import FormMixin
from django.contrib.messages.views import SuccessMessageMixin

reveal_type(FormMixin.form_valid)
# Revealed type is 'def (self: ..., form: django.forms.forms.BaseForm) -> django.http.response.HttpResponse'
reveal_type(SuccessMessageMixin.form_valid)
# Revealed type is 'def (self: ..., form: django.forms.forms.Form) -> django.http.response.HttpResponseRedirect'

class FooView(FormMixin, SuccessMessageMixin):
    pass

# Definition of "form_valid" in base class "FormMixin" is incompatible with definition in base class "SuccessMessageMixin"

I suspect that the signature of FormMixin is the one we want to use, so the fix is to update SuccessMessageMixin to match. (If you agree that's a good solution I'd be happy to put up a PR)

@antonagestam
Copy link
Contributor

Yes, I believe restricting SuccessMessageMixin.form_valid to take Form instead of BaseForm violates the Liskov substitution principle.

PeterJCLaw referenced this issue in PeterJCLaw/django-stubs Jun 2, 2019
This ensures that the order in which these mixins are included
into a derrived class does not matter and ends up more accurately
reflecting the return type of SuccessMessageMixin in the process
(its code doesn't appear to enforce that the returned response
is a redirect).

This provides a fix to a secondary aspect of
https://github.com/mkurnikov/django-stubs/issues/79.
@PeterJCLaw
Copy link
Contributor Author

Unfortunately that fix doesn't seem to completely resolve the issue for me. I'll put up a PR with a proposed fix.

@antonagestam
Copy link
Contributor

@PeterJCLaw Oh yeah, that makes sense I guess.

Is there a way to use an ABC to make sure view-mixins have the right shape in typestubs?

@PeterJCLaw
Copy link
Contributor Author

I'm not sure if that's possible in type stubs. I'd guess it probably is, though not a pattern I've seen.

I'm guessing you mean something like this:

# common.pyi
class _FormMixinCommon(abc.ABC):
    def form_valid(self, ...): ...

# forms.pyi
from ...common import _FormMixinCommon
class FormMixin(_FormMixinCommon):
    ...

# forms.pyi
from ...common import _FormMixinCommon
class SuccessMessageMixin(_FormMixinCommon):
    ...

I'm not sure whether that's any better than just duplicating the signature and adding a test (the latter being what I've done in that PR).

I suspect that duplicating the signature is probably clearer and (at least for small numbers of duplicates) doesn't represent much maintenance burden.

There's probably benefit in keeping the type definitions reasonably close to the shape of the actual source we're typing.

Are there other places where you think the ABC pattern might be useful? (If it just ends up being this place I suspect it would be more confusing to readers.)

mkurnikov referenced this issue Jul 2, 2019
This ensures that the order in which these mixins are included
into a derrived class does not matter and ends up more accurately
reflecting the return type of SuccessMessageMixin in the process
(its code doesn't appear to enforce that the returned response
is a redirect).

This provides a fix to a secondary aspect of
https://github.com/mkurnikov/django-stubs/issues/79.
@TonyRippy TonyRippy mentioned this issue Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants