Set HTTP client timeout using FACTURAPI_TIMEOUT environment variable#121
Conversation
WalkthroughThis PR introduces a configurable HTTP timeout for the Facturapi client and bumps the version to 1.0.1. The Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
facturapi/http/client.py (1)
12-12: Note: Timeout is evaluated at module import time.The
FACTURAPI_TIMEOUTconstant is read once when the module is imported. Runtime changes to the environment variable will not be reflected until the module is reloaded. This is likely intentional for performance, but worth documenting if this behavior matters for your use case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@facturapi/http/client.py` at line 12, FACTURAPI_TIMEOUT is read once at module import and won't reflect later env var changes; either document this behavior in the module-level docstring (or README) near FACTURAPI_TIMEOUT to state it is evaluated at import time, or change to a runtime-read approach (e.g., replace the constant with a helper function like get_facturapi_timeout() that reads os.getenv each call) so callers can opt into dynamic behavior; reference the FACTURAPI_TIMEOUT constant in facturapi/http/client.py when making the update.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@facturapi/http/client.py`:
- Line 12: The module-level conversion of FACTURAPI_TIMEOUT using
float(os.getenv('FACTURAPI_TIMEOUT', 10.0)) can raise ValueError on import if
the env var is non-numeric; update the code that defines FACTURAPI_TIMEOUT to
validate the environment value by reading os.getenv('FACTURAPI_TIMEOUT'),
attempting float conversion inside a try/except, falling back to the default
10.0 on failure (and optionally logging or raising a clear, descriptive error),
and ensure the constant FACTURAPI_TIMEOUT is always set to a valid float.
---
Nitpick comments:
In `@facturapi/http/client.py`:
- Line 12: FACTURAPI_TIMEOUT is read once at module import and won't reflect
later env var changes; either document this behavior in the module-level
docstring (or README) near FACTURAPI_TIMEOUT to state it is evaluated at import
time, or change to a runtime-read approach (e.g., replace the constant with a
helper function like get_facturapi_timeout() that reads os.getenv each call) so
callers can opt into dynamic behavior; reference the FACTURAPI_TIMEOUT constant
in facturapi/http/client.py when making the update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e27a2ad-c88d-4023-8e8f-c708959ebf77
📒 Files selected for processing (2)
facturapi/http/client.pyfacturapi/version.py
| from ..version import CLIENT_VERSION | ||
|
|
||
| API_HOST = 'www.facturapi.io/v2' | ||
| FACTURAPI_TIMEOUT = float(os.getenv('FACTURAPI_TIMEOUT', 10.0)) |
There was a problem hiding this comment.
Add validation for the FACTURAPI_TIMEOUT environment variable.
If FACTURAPI_TIMEOUT is set to a non-numeric value, float() will raise a ValueError at module import time, crashing the application. Consider adding error handling to validate the input or provide a clearer error message.
🛡️ Proposed fix to add validation
-FACTURAPI_TIMEOUT = float(os.getenv('FACTURAPI_TIMEOUT', 10.0))
+try:
+ FACTURAPI_TIMEOUT = float(os.getenv('FACTURAPI_TIMEOUT', '10.0'))
+except ValueError:
+ raise ValueError(
+ "FACTURAPI_TIMEOUT environment variable must be a valid number"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FACTURAPI_TIMEOUT = float(os.getenv('FACTURAPI_TIMEOUT', 10.0)) | |
| try: | |
| FACTURAPI_TIMEOUT = float(os.getenv('FACTURAPI_TIMEOUT', '10.0')) | |
| except ValueError: | |
| raise ValueError( | |
| "FACTURAPI_TIMEOUT environment variable must be a valid number" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@facturapi/http/client.py` at line 12, The module-level conversion of
FACTURAPI_TIMEOUT using float(os.getenv('FACTURAPI_TIMEOUT', 10.0)) can raise
ValueError on import if the env var is non-numeric; update the code that defines
FACTURAPI_TIMEOUT to validate the environment value by reading
os.getenv('FACTURAPI_TIMEOUT'), attempting float conversion inside a try/except,
falling back to the default 10.0 on failure (and optionally logging or raising a
clear, descriptive error), and ensure the constant FACTURAPI_TIMEOUT is always
set to a valid float.
2 participants
Summary by CodeRabbit
FACTURAPI_TIMEOUTenvironment variable (default: 10 seconds).