-
Notifications
You must be signed in to change notification settings - Fork 1
[RTC-409] Switch openapi generator #21
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
Conversation
375e31f to
d077fce
Compare
d077fce to
14051ea
Compare
0ad32ac to
a2defad
Compare
jellyfish/_room_api.py
Outdated
| def _request(self, method, **kwargs): | ||
| resp = method.sync(client=self._client, **kwargs) | ||
| if isinstance(resp, Error): | ||
| raise RuntimeError(resp.errors) |
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.
@Rados13 Do you think it's ok to just throw a RuntimeError if a request should fail?
Before we had a couple more specific Exception types, such as UnauthorizedException, NotFoundException, BadRequestException
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 think that a specific Exception would be better.
jellyfish/_room_api.py
Outdated
| def _request(self, method, **kwargs): | ||
| resp = method.sync(client=self._client, **kwargs) | ||
| if isinstance(resp, Error): | ||
| raise RuntimeError(resp.errors) |
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 think that a specific Exception would be better.
Karolk99
left a comment
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.
"In all API classes, the _request method is repeated. I suggest creating a class with a @staticmethod request, or alternatively, create a class that inherits from it."
6c338ff to
6d351b9
Compare
poetry_scripts.py
Outdated
| import pdoc as p | ||
| from pdoc import render | ||
|
|
||
| # ruff: noqa: E501 |
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.
Ignore line length in this 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.
Why do we need this?
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.
Ok, I fixed the string that was too long.
| ignore = [] | ||
|
|
||
| [tool.ruff.extend-per-file-ignores] | ||
| "jellyfish/_openapi_client/**" = ["E501"] |
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.
Ignore line length for autogenerated files - some comments are too long and cannot be automatically wrapped.
jellyfish/_webhook_notifier.py
Outdated
|
|
||
|
|
||
| def receive_json(json: Dict) -> Union[server_messages]: | ||
| def receive_json(json: Dict) -> Union[SERVER_MESSAGE_TYPES]: |
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.
When I generate docs locally I get warnings like this:
Warn: Error parsing type annotation typing.Union[ForwardRef('ServerMessageComponentCrashed'), ForwardRef('ServerMessageHlsPlayable'), ForwardRef('ServerMessageMetricsReport'), ForwardRef('ServerMessagePeerConnected'), ForwardRef('ServerMessagePeerCrashed'), ForwardRef('ServerMessagePeerDisconnected'), ForwardRef('ServerMessageRoomCrashed'), ForwardRef('ServerMessageRoomCreated'), ForwardRef('ServerMessageRoomDeleted')] for jellyfish.receive_json. Import of ServerMessageComponentCrashed failed: name 'ServerMessageComponentCrashed' is not defined
Should it look like this? Also maybe we should add action to check if there are any warnings during docs generation?
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.
Ok, just changed the return type of receive_json to resolve this shenanigans.
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.
If warning is generated the exit code is still 0, and there is no --warning-as-errors flag. We could capture the output and check if there are any warnings, or if there is any output at all (there shouldn't be any).
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.
We probably could solve this by using env PYTHONWARNINGS=error as suggested by pdoc maintainer in this issue.
poetry_scripts.py
Outdated
| import pdoc as p | ||
| from pdoc import render | ||
|
|
||
| # ruff: noqa: E501 |
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.
Why do we need this?
Most changes include new models in
_openapi_clientand code formatting - the're probably not worth looking at.Take a look at:
_room_api.py,_recording_api.py- minor changes related to the new apipyproject.tomlpoetry_scripts.py_webhook_notifier.pyThe tests fail, since there have been some updates in Jellyfish. The related changes will follow in the next PR.