Skip to content
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

Exposed response_timeout parameter in open_connection methods #138

Merged

Conversation

marcindebski
Copy link
Collaborator

No description provided.

@marcindebski marcindebski requested a review from a team July 25, 2024 13:14
@marcindebski marcindebski self-assigned this Jul 25, 2024
@marcindebski marcindebski requested review from skamieniarz and removed request for a team July 25, 2024 13:14
changelog.md Outdated
@@ -8,6 +8,8 @@ All notable changes to this project will be documented in this file.

### Changed
- Updated outdated packages.
- Exposed `response_timeout` parameter in `open_connection` methods.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Exposed" indicates that it was already present but hidden. I'd consider moving it to "Added" section and rephrasing to:

"Added response_timeout parameter in open_connection methods."

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to Cambridge Dictionary: "expose: to make something covered or hidden able to seen". Well, I would say that response_timeout was somewhere available in the code but covered by the encapsulation within the structure of the SDK. Anyway, it is all the same to me, so I will apply your suggestion.

ping_interval: float = 5,
ws_conn_timeout: float = 10,
keep_alive: bool = True) -> None:
keep_alive: bool = True,
response_timeout: float = 3) -> None:
Copy link
Collaborator

@skamieniarz skamieniarz Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems strange to me that all values have the type marked as float and the default value as int. Since we are making changes in this area, I would consider changing all types to:

Suggested change
response_timeout: float = 3) -> None:
response_timeout: Union[float, int] = 3) -> None:

considering (int or float) in the docstring for response_timeout arg and https://websocket-client.readthedocs.io/en/latest/app.html#websocket._app.WebSocketApp.run_forever

ping_interval (int or float), ping_timeout (int or float)

ws_conn_timeout should stay as float with 10.0 as default value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Therefore I have chosen float to be consistent with others. Anyway, I will change all types to Union[ float, int].

@marcindebski marcindebski merged commit 75bf5de into master Jul 25, 2024
1 check passed
@marcindebski marcindebski deleted the Expose_response_timeout_in_open_connection_method branch July 25, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants