feat: allow float timeouts in the clientconfig#2671
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2671 +/- ##
==========================================
- Coverage 93.59% 93.55% -0.05%
==========================================
Files 492 492
Lines 49946 49946
==========================================
- Hits 46748 46727 -21
- Misses 3198 3219 +21
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request updates the type hint of the timeout parameter in ClientConfig from int | None to float | None and adds a float timeout to the integration test client. The review feedback highlights a correctness issue where a timeout of 0.0 would be incorrectly overridden to 60 due to a falsy check, and suggests updating file_transfer_timeout to also support floats. Additionally, the reviewer recommends adding a dedicated unit test for the float timeout configuration instead of relying on the global integration test setup.
| cluster: str | None = None, | ||
| headers: dict[str, str] | None = None, | ||
| timeout: int | None = None, | ||
| timeout: float | None = None, |
There was a problem hiding this comment.
While updating the type hint of timeout to float | None, please note that the current implementation in __init__ uses self.timeout = timeout or 60. Since 0.0 is a falsy value in Python, passing timeout=0.0 (which is a valid float timeout for non-blocking/immediate timeout) will be incorrectly overridden to 60.\n\nTo fix this correctness issue, the assignment should be updated to:\npython\nself.timeout = 60 if timeout is None else timeout\n\n\nAdditionally, for consistency and completeness, file_transfer_timeout should also be updated to support float | None instead of strictly int | None.
References
- Strong Typing: Use type hints extensively with MyPy. Avoid Any when possible (link)
| project=os.environ["COGNITE_PROJECT"], | ||
| base_url=os.environ["COGNITE_BASE_URL"], | ||
| credentials=credentials, | ||
| timeout=59.9, # Test that a non-int timeout works |
There was a problem hiding this comment.
Relying on the global integration test client configuration to verify that float timeouts work is not a robust testing strategy. It doesn't explicitly assert the behavior and could be easily lost or modified in the future.\n\nPlease add a dedicated unit test (e.g., in tests/tests_unit/test_config.py) that instantiates ClientConfig with a float timeout and asserts that client.config.timeout is correctly set to the float value. This ensures explicit test coverage and prevents regressions.
I made this PR because I wanted to set timeout < 1 s. Useful when e.g. troubleshooting authentication issues in a CDF project you know should respond quickly. |
Description
Change the timeout type hint in the
ClientConfigto show that non-int, float timouts works.Checklist:
If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.