Feature/api rate control #6
Conversation
📝 WalkthroughWalkthroughThis pull request introduces comprehensive improvements to the pyRail library, focusing on development environment configuration, documentation enhancement, and API client functionality. The changes include adding a development container configuration, removing the GitLab CI file, significantly expanding the README documentation, refactoring the iRail API client with advanced rate limiting and logging features, and updating the test suite to support the new implementation. Changes
Sequence DiagramsequenceDiagram
participant Client
participant iRail
participant API
Client->>iRail: Create instance
Client->>iRail: do_request()
iRail->>iRail: Check token availability
iRail->>API: Send request
API-->>iRail: Return response
iRail->>iRail: Cache ETag
iRail-->>Client: Return response data
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
pyrail/irail.py (6)
1-3: Consider deferring logging configuration to application code.
Callinglogging.basicConfigin a library file can override or conflict with logging settings in other parts of the application. It might be better to configure logging in the main entry point or a dedicated configuration file.
57-72: Refill tokens logic: consider negativetime.sleepscenarios.
In_refill_tokensand related code, consecutive calls totime.sleep(1 - (time.time() - self.last_request_time))may yield a negative value if the time gap is large, resulting in an immediate return instead of a wait. To handle edge cases gracefully, usemax(0, 1 - (time.time() - self.last_request_time)).- time.sleep(1 - (time.time() - self.last_request_time)) + wait_time = 1 - (time.time() - self.last_request_time) + time.sleep(max(0, wait_time))
74-88: Potential indefinite retry scenario.
When tokens are depleted, the logic repeatedly sleeps and refills. In a high-load environment, repeated 429 responses could cause prolonged loops. Consider bounding the total wait time or number of iterations to avoid indefinite blocking.
94-100: Add ETag usage to logs for clarity.
You're already logging ETag additions; consider informing the user when ETag is set or used, for more transparent caching diagnostics. This helps debug scenarios where stale or missing ETags cause unexpected responses.
103-122: Enhance rate-limited retry logic.
The current approach retries once after waiting forRetry-After. If the server continues returning 429, it may loop repeatedly. Implement a maximum retry count or exponential backoff to avoid prolonged retries.
124-131: Improve exception handling.
When arequests.exceptions.RequestExceptionoccurs, you try a network call tohttps://1.1.1.1/. This is a neat connectivity check, but can introduce external dependencies at runtime. Consider gracefully handling unreachable cases or using a more robust approach (e.g., DNS or local circuit check) if reliability is paramount.Do you want me to propose an updated error-handling workflow that avoids external calls for checking connectivity?
tests/test_irail.py (1)
7-23: Extensive test approach with mocks.
Usingunittest.mockto simulate HTTP responses is a dependable approach. Verify additional scenarios (e.g., 429 or 304 codes) in separate tests to ensure full rate-limiting coverage..gitlab-ci.yml (2)
4-4: Add a stage for linting or static analysis.
You might consider adding a separate stage for linting or static analysis to catch issues early before running tests.
26-27: No new line at end of file.
yaml files often require a newline at the end of file. Consider adding it to satisfy linting tools like yamllint.... rules: - if: $CI_COMMIT_BRANCH == "main" # Deploy only when merging into the main branch +🧰 Tools
🪛 yamllint (1.35.1)
[error] 27-27: no new line character at the end of file
(new-line-at-end-of-file)
README.md (3)
5-8: Mention rate limiting in top-level descriptions.
You have an excellent sentence about caching and rate limiting. Consider giving it more prominence by referencing best practices (e.g., “Uses a token bucket approach for rate limiting”).
17-31: Usage example matches new rate-limiting features.
The snippet effectively demonstrates how to initialize and call an endpoint. Including a short mention of possible 429 responses or caching usage could guide users even better.
51-58: Clarify dynamic logging configuration.
Your code callslogging.basicConfiginside the library, which can conflict with instructions to set logging levels at runtime. Indicating that the user might only need to callset_logging_levelif the global config is not changed could prevent confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.devcontainer/devcontainer.json(1 hunks).gitlab-ci.yml(1 hunks)README.md(1 hunks)pyrail/irail.py(3 hunks)requirements.txt(1 hunks)tests/__init__.py(1 hunks)tests/test_irail.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- requirements.txt
- .devcontainer/devcontainer.json
🧰 Additional context used
🪛 yamllint (1.35.1)
.gitlab-ci.yml
[error] 27-27: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Ruff (0.8.2)
tests/__init__.py
1-1: pyrail.irail.irail imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
🔇 Additional comments (10)
pyrail/irail.py (1)
25-33: Tokens and burst tokens logic: verify partial token increments.
The current approach allows fractional tokens via elapsed * 3. This can lead to partial tokens if elapsed is not an integer, which might not align with your intended rate-limiting policy. Ensure partial token usage is acceptable, or consider rounding logic if integer tokens are desired.
tests/test_irail.py (2)
1-3: Test coverage looks good.
Importing iRail directly ensures you’re targeting the same logic the end user would exercise. This is standard and recommended practice.
25-26: Automatic test discovery.
The if __name__ == '__main__': unittest.main() pattern is standard for direct test execution. Integration with CI is straightforward. Tests appear well structured.
.gitlab-ci.yml (2)
8-8: Consolidate dependency installation.
Installing from requirements.txt ensures all dependencies are pinned and consistent. Confirm that requests, unittest.mock, and any additional libraries are present in the file.
14-20: Test stage rules look good.
Running tests on every merge request fosters reliable code. Good job enabling test runs only when CI_MERGE_REQUEST_ID is set.
README.md (5)
3-3: Concise introduction.
The overview is clear and precisely describes the library’s purpose. Great job providing immediate context for new users.
9-15: Installation instructions are straightforward.
Providing a pip command is enough for typical usage. Nicely done.
33-50: Feature highlights are thorough.
Great job enumerating endpoints, caching, ETag, and rate limiting. This ensures new users see exactly what the library supports.
59-65: Appreciation for contributors.
A dedicated contributors list is a nice, welcoming touch.
66-70: Clear license notice.
MIT license is widely recognized. The mention in README fosters transparency for open-source usage.
| @@ -0,0 +1 @@ | |||
| from pyrail.irail import irail | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unused import.
According to static analysis, from pyrail.irail import irail is unused. Remove or alias it to avoid confusion.
-from pyrail.irail import irail🧰 Tools
🪛 Ruff (0.8.2)
1-1: pyrail.irail.irail imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
|
Hi @jcoetsie, I'll reply to your email after the holidays. Thanks for taking the effort to help with this library. As you might see in the commit history this was more of a weekend project to learn about python libraries. It was never intended to be used for something like a HA integration. We should probably just continue development here on github instead of gitlab. Some things are already updated to modern standards and tools like poetry. GitHub action will need to be added as well (e.g. for publishing). I changed the license from MIT to Apache 2.0, is this alright for you? |
Adding api rate limiting & caching as specified in https://docs.irail.be
Summary by CodeRabbit
New Features
Documentation
Refactor
iRailclassChores