Make InertiaRequest inherit from HttpRequest (#78) #84
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Thanks @kennyputman ! Per the copilot review comment, now that we're extending |
|
Thanks for checking that out @BrandonShar. I updated the code to use inheritance with Do you have any thoughts on this? |
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the InertiaRequest class to inherit directly from Django's HttpRequest instead of using composition with a __getattr__ delegation pattern. This change improves compatibility with other Django libraries that expect request objects to be instances of HttpRequest.
- Changes
InertiaRequestfrom composition to inheritance pattern - Updates property methods to use proper inheritance approach
- Replaces attribute delegation with direct dictionary copying
| def __getattr__(self, name): | ||
| return getattr(self.request, name) | ||
| super().__init__() | ||
| self.__dict__.update(request.__dict__) |
There was a problem hiding this comment.
nice, this is a clean way of handling this; I thought it might be more complex.
There was a problem hiding this comment.
@kennyputman this looks great, thanks for fixing it up! Just a couple minor changes and I think we're good to merge!
inertia/http.py
Outdated
|
|
||
| @property | ||
| def headers(self): | ||
| return self.request.headers | ||
| return super().headers |
There was a problem hiding this comment.
It looks like we can remove this method entirely now, right?
There was a problem hiding this comment.
Definitely, thanks for the catch!
| return self.request.inertia.all() if hasattr(self.request, "inertia") else {} | ||
| inertia_attr = self.__dict__.get("inertia") | ||
| return ( | ||
| inertia_attr.all() if inertia_attr and hasattr(inertia_attr, "all") else {} |
There was a problem hiding this comment.
It looks like the refactor equivalent should be something like
return self.inertia.all() if hasattr(self, "inertia") else {}does that look right?
There was a problem hiding this comment.
I think I originally tried something like that, but it ends up in a RecursionError as the property repeatedly calls itself.
The current code is a bit of a workaround as it seems the more idiomatic way is to rename the attribute to self._inertia, but that would require updating share.py. It could be done with something similar to this.
def share(request, **kwargs):
if not hasattr(request, "_inertia"):
request._inertia = InertiaShare()
request._inertia.set(**kwargs)Then it could be called using
return self._inertia.all() if hasattr(self, "_inertia") else {}Do you want me to make these updates or stick to the current code?
There was a problem hiding this comment.
ahhh yeah I get that. Ok, I think this makes sense and maybe we'll come back to it later. Thanks for double checking!
kennyputman
force-pushed
the
fix-inertiarequest-inherits-httprequest
branch
from
714d934 to
e372e11
Compare
Follow up on #78. Makes
InertiaRequestinherit from Django'sHttpRequestin order to improve compatibility with other libraries.