-
Notifications
You must be signed in to change notification settings - Fork 15
allow specifying expected response type #58
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
base: develop
Are you sure you want to change the base?
Conversation
| BYTES = "bytes" | ||
| NO_CONTENT = "no_content" | ||
|
|
||
| ResponseTypeLiteral = Literal["json", "text", "bytes", "no_content"] No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need this literal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's more convenient and faster, we remove one useless import
src/dataclass_rest/rest.py
Outdated
| additional_params: Optional[Dict[str, Any]] = None, | ||
| method_class: Optional[Callable[..., BoundMethod]] = None, | ||
| send_json: bool = True, | ||
| response_type: Optional[ResponseTypeLiteral] = "json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use enum here? anyway I do not see why we should allow None value
response_type: ResponseType = ResponseType.json,There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, it's more convenient to use a literal. This value is optional and by default it's json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do not handle Nine value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/dataclass_rest/http/aiohttp.py
Outdated
| case ResponseType.BYTES: | ||
| return await response.read() | ||
| case ResponseType.NO_CONTENT: | ||
| return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we validate content-length here?
I'd rather have 2 cases here:
- SKIP
- NO_CONTENT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- content-length I think validation has a place to be, but shouldn't it be at the HTTP level?
(I think we should ask the community) - I think it's unnecessary, maybe it will confuse
src/dataclass_rest/http/aiohttp.py
Outdated
| async def _response_body(self, response: ClientResponse) -> Any: | ||
| try: | ||
| return await response.json() | ||
| match self.response_type: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's avoid match for now, just in case we want to supper older python
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this can be fixed.
|



No description provided.