fix: compatible with urllib@2 timeout string format #580
Conversation
WalkthroughThe changes update the handling of the Changes
Assessment against linked issues
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/HttpClient.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. test/options.timeout.test.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. Warning Review ran into problems🔥 ProblemsCheck-run timed out after 90 seconds. Some checks/pipelines were still in progress when the timeout was reached. Consider increasing the reviews.tools.github-checks.timeout_ms value in your CodeRabbit configuration to allow more time for checks to complete. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @fengmk2, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses a compatibility issue within the HttpClient by enhancing its timeout handling mechanism. It introduces the ability to correctly interpret and process timeout values provided as strings, aligning its behavior with external libraries like urllib@2. A new test case has been added to confirm the robustness and correctness of this updated timeout parsing logic.
Highlights
- Timeout Handling Compatibility: Modified the
HttpClientto correctly parse string-formatted timeout values by converting them to integers usingparseInt(). This ensures compatibility withurllib@2's expected timeout string format, preventing potential issues whentimeoutis provided as a string instead of a number or tuple. - New Test Coverage: Added a new test case (
should timeout support string format) totest/options.timeout.test.ts. This test specifically verifies that passing a string value for thetimeoutoption correctly triggers a timeout and that the resulting error details (name, message, cause) are as expected, validating the implemented compatibility fix.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR adds compatibility for string-formatted timeout values by parsing them into integers and includes a corresponding test.
- Update HttpClient to parse string timeouts for urllib@2 compatibility
- Add a new test case to verify timeout behavior when provided as a string
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/options.timeout.test.ts | Added test verifying string-format timeout handling |
| src/HttpClient.ts | Parse string timeout values to integer for compatibility |
Comments suppressed due to low confidence (1)
test/options.timeout.test.ts:44
- [nitpick] The test title is a bit awkward; consider renaming it to "should support timeout as a string format" for clarity.
it('should timeout support string format', async () => {
commit: |
There was a problem hiding this comment.
Code Review
This pull request adds compatibility for string-based timeout values, as was supported in urllib@2. The implementation correctly handles string-to-number conversion, and a new test case validates this behavior. I've made one suggestion to improve the robustness of the string parsing by explicitly specifying the radix. Overall, the change is clear and well-tested.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #580 +/- ##
=======================================
Coverage 95.64% 95.64%
=======================================
Files 11 11
Lines 1355 1355
Branches 312 313 +1
=======================================
Hits 1296 1296
Misses 55 55
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/HttpClient.ts (1)
372-373: Improve robustness of string timeout parsing.The implementation correctly addresses the compatibility requirement, but could be more robust in handling edge cases.
Consider the following improvements:
- // compatible with urllib@2 timeout string format - headersTimeout = bodyTimeout = typeof args.timeout === 'string' ? parseInt(args.timeout) : args.timeout; + // compatible with urllib@2 timeout string format + headersTimeout = bodyTimeout = typeof args.timeout === 'string' ? parseInt(args.timeout, 10) : args.timeout;Rationale:
- Specifying radix
10ensures consistent decimal parsing- However, consider if additional validation is needed for invalid string formats (e.g.,
parseInt('abc', 10)returnsNaN)Since this is for urllib@2 compatibility, the current approach might be intentionally minimal to match the expected behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/HttpClient.ts(1 hunks)test/options.timeout.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/options.timeout.test.ts (1)
test/cjs/index.js (2)
assert(2-2)urllib(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Node.js / Test (windows-latest, 16)
- GitHub Check: Node.js / Test (macos-latest, 22)
- GitHub Check: Node.js / Test (macos-latest, 18)
- GitHub Check: Node.js / Test (windows-latest, 18)
- GitHub Check: Node.js / Test (macos-latest, 16)
- GitHub Check: Node.js / Test (macos-latest, 24)
- GitHub Check: Node.js / Test (ubuntu-latest, 16)
- GitHub Check: Node.js / Test (windows-latest, 24)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (macos-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (ubuntu-latest, 24)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
🔇 Additional comments (1)
test/options.timeout.test.ts (1)
44-63: Excellent test coverage for string timeout compatibility.The test case effectively validates the string timeout format compatibility:
- Mirrors the existing numeric timeout test structure
- Verifies identical error behavior for both string and numeric inputs
- Includes appropriate TypeScript error handling with
@ts-expect-errorThe test confirms that
timeout: '10'produces the sameHttpClientRequestTimeoutErrorwith identical properties astimeout: 10, ensuring backward compatibility is maintained.
[skip ci] ## [4.7.1](v4.7.0...v4.7.1) (2025-07-07) ### Bug Fixes * compatible with urllib@2 timeout string format ([#580](#580)) ([5eaf790](5eaf790))
close hyj1991/easy-monitor#166
Summary by CodeRabbit
Bug Fixes
Tests