[engine] Fix some type hints when request is passed
#9077
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #9077 +/- ##
===========================================
+ Coverage 73.75% 73.92% +0.17%
===========================================
Files 428 429 +1
Lines 44514 44546 +32
Branches 3881 3881
===========================================
+ Hits 32830 32932 +102
+ Misses 11684 11614 -70
|
cvat/apps/engine/types.py
Outdated
| from cvat.apps.iam.middleware import WithIAMContext | ||
|
|
||
|
|
||
| class PatchedRequest(HttpRequest, Request, WithUUID, WithIAMContext): |
There was a problem hiding this comment.
I think PatchedRequest gives off the wrong idea. "Patched" implies some sort of change, but we only add new attributes. How about something like ExtendedRequest or AugmentedRequest?
There was a problem hiding this comment.
How about just engine.types.Request?
There was a problem hiding this comment.
How about just
engine.types.Request?
IMO, it's too ambiguous. But if both you and @Marishka17 prefer it, I'm not going to object.
There was a problem hiding this comment.
I mean, it should be something like HttpRequest or ApiRequest, but we need to avoid confusion with other requests we already have, such as function requests or async operation requests. I'm not sure this module (called types) is a good place for such an abstraction, maybe it should be something server API or http related (e.g. we already have view_uitls).
There was a problem hiding this comment.
HttpRequest would be ambiguous too (since it's the name of the base Django type).
| if request.method == 'OPTIONS': | ||
| return self._tus_response(status=status.HTTP_204) | ||
| return self._tus_response(status=status.HTTP_204_NO_CONTENT) |
There was a problem hiding this comment.
That's interesting... was this broken before? If so, you should probably add a changelog entry, as this is a user-visible change.
There was a problem hiding this comment.
Mind adding the changelog entry, then?
|
Motivation and context
Since we are adding custom attributes to the request object (such as
uuidoriam_context), using theRequesttype is no longer appropriate.Based on #9075
How has this been tested?
Checklist
developbranch- [ ] I have created a changelog fragment- [ ] I have updated the documentation accordingly- [ ] I have added tests to cover my changes- [ ] I have linked related issues (see GitHub docs)License
Feel free to contact the maintainers if that's a concern.