fix: Host string parsing #1204
Merged
fix: Host string parsing #1204
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Changed the test_set_by_netrc function to use 'https://example.ai' instead of 'https://example.cn' for host verification. This aligns the test with the expected domain for SwanLab environment configuration.
|
THX! |
Moved host suffix removal logic to a dedicated function `remove_host_suffix` in env.py and updated its usage in package.py. This improves code reuse and consistency when handling host strings.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in host string parsing where rstrip("api") was incorrectly treating its argument as a character set, causing domains ending with .ai, .app, etc. to be truncated. The fix introduces a new remove_host_suffix function that properly removes exact suffix matches.
- Replaces problematic
rstrip("api")calls with a newremove_host_suffixfunction - Adds comprehensive test coverage for the new suffix removal functionality
- Updates existing test cases to use
.aidomains to demonstrate the fix
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| swanlab/env.py | Implements new remove_host_suffix function and updates host parsing logic |
| swanlab/package.py | Replaces rstrip calls with remove_host_suffix function calls |
| test/unit/test_env.py | Adds comprehensive tests for suffix removal and updates existing tests to use .ai domains |
| test/unit/test_package.py | Updates test cases but uses incorrect hardcoded slicing approach |
Comments suppressed due to low confidence (2)
test/unit/test_env.py:101
- The test name "test_handles_host_with_blank_suffix" is misleading. The test actually checks a host with trailing whitespace and a suffix that doesn't match. Consider renaming to "test_handles_host_with_whitespace_and_non_matching_suffix" to better reflect what is being tested.
def test_handles_host_with_blank_suffix():
test/unit/test_env.py:104
- This test case doesn't clearly validate the intended behavior. With host="example.com " and suffix="api", the expected result "example.com" suggests whitespace trimming, but the suffix "api" doesn't match the host ending. Consider adding a clearer test case that validates whitespace trimming behavior separately.
assert remove_host_suffix(host, suffix) == "example.com"
Replaces manual host suffix removal with the remove_host_suffix utility in TestSaveKey. Also updates the API_HOST environment variable assignment in TestGetKey for consistency.
SAKURA-CAT
approved these changes
Jul 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Replace
rstrip("api")with an exact"/api"suffix removal to avoid stripping valid domain endings like.ai,.app, etc.rstrip("api")treats its argument as a character set and removes all trailinga/p/i, causing hosts such aswww.example.aito becomewww.example..The fix usesstr.removesuffix("/api")(Py≥3.9) with a safe fallback for older Python versions.The patch calls removesuffix("/api") on Python 3.9+ and falls back to simple slicing ([:-4]) on earlier python versions.
New tests demonstrate the incorrect behavior and verify the corrected result.
Closes: #1203
🎯 PRs Should Target Issues
This PR is tied to the bug report above. If a different issue ID is used, update the
Closes:line accordingly.Changes
rstrip("api")with an exact"/api"suffix removal..aidomain not being truncated"/api"suffix removal/apip,/appi, etc.) left untouched"/api"stripped when repeated