Conversation
dosubot
bot
added
the
size:S
|
WalkthroughRuff pre-commit hook version bumped from v0.14.14 to v0.15.2. Host URL construction normalization simplified in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #173 +/- ##
=======================================
Coverage 86.64% 86.64%
=======================================
Files 8 8
Lines 749 749
=======================================
Hits 649 649
Misses 100 100 ☔ 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@airos/base.py`:
- Around line 60-64: The current construction of self.base_url uses
parsed_host.hostname which drops explicit ports; update the logic in the url
parsing block (symbols: urlparse, parsed_host, scheme, use_ssl, hostname,
base_url) to use parsed_host.netloc (falling back to host if netloc is empty)
when composing the authority portion so ports are preserved; keep the existing
scheme selection (parsed_host.scheme or ("https" if use_ssl else "http")) and
then set self.base_url = f"{scheme}://{netloc}" (with netloc defaulting to host
when needed).
| parsed_host = urlparse(host) | ||
| scheme = ( | ||
| parsed_host.scheme | ||
| if parsed_host.scheme | ||
| else ("https" if use_ssl else "http") | ||
| ) | ||
| hostname = parsed_host.hostname if parsed_host.hostname else host | ||
| scheme = parsed_host.scheme or ("https" if use_ssl else "http") | ||
| hostname = parsed_host.hostname or host | ||
|
|
||
| self.base_url = f"{scheme}://{hostname}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find airos/base.py and read the relevant lines
git ls-files | grep -E 'airos.*base\.py'Repository: CoMPaTech/python-airos
Length of output: 79
🏁 Script executed:
#!/bin/bash
# Read the code around lines 60-64 in airos/base.py to see exact implementation
find . -name 'base.py' -path '*/airos/*' -type f | head -5Repository: CoMPaTech/python-airos
Length of output: 81
🏁 Script executed:
#!/bin/bash
# Search for how this class is instantiated with host parameter
rg -n 'class.*\(.*\):' airos/ | grep -i baseRepository: CoMPaTech/python-airos
Length of output: 345
🏁 Script executed:
#!/bin/bash
# Read the __init__ method and surrounding code in airos/base.py
sed -n '39,100p' airos/base.py | cat -nRepository: CoMPaTech/python-airos
Length of output: 2739
🏁 Script executed:
#!/bin/bash
# Search for instantiation of AirOS class with host parameter
rg -n 'AirOS\(' -A 2 --type=py | head -50Repository: CoMPaTech/python-airos
Length of output: 194
🏁 Script executed:
#!/bin/bash
# Also search for test files that show how this class is used
find . -name '*test*.py' -type f | xargs grep -l 'AirOS' | head -3Repository: CoMPaTech/python-airos
Length of output: 131
🏁 Script executed:
#!/bin/bash
# Read test files to see how host is passed
cat tests/conftest.py | head -100Repository: CoMPaTech/python-airos
Length of output: 2155
🏁 Script executed:
#!/bin/bash
# Search for host parameter values in tests
rg -n 'host\s*=' tests/ -A 1 -B 1Repository: CoMPaTech/python-airos
Length of output: 1650
🏁 Script executed:
#!/bin/bash
# Search for AirOS instantiation in tests showing host parameter
rg -n 'AirOS\(' tests/ -A 5 | head -80Repository: CoMPaTech/python-airos
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Search for any host parameter with explicit port numbers in the codebase
rg -n ':\d+' airos/ tests/ --type=py | grep -E '(host|url).*:\d+'Repository: CoMPaTech/python-airos
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or examples showing port usage
find . -name 'README*' -o -name '*.md' | xargs grep -l 'port\|:\d\d\d\d' 2>/dev/null | head -5Repository: CoMPaTech/python-airos
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Let's verify the urlparse behavior with a quick Python test
python3 << 'EOF'
from urllib.parse import urlparse
# Test cases showing the difference between hostname and netloc
test_cases = [
"192.168.1.3",
"http://192.168.1.3",
"https://example.com:8443",
"http://device.local:9000",
"device.local",
]
print("URL | hostname | netloc")
print("-" * 60)
for url in test_cases:
parsed = urlparse(url)
print(f"{url:30} | {parsed.hostname or 'None':20} | {parsed.netloc or 'None'}")
EOFRepository: CoMPaTech/python-airos
Length of output: 487
Preserve explicit ports in base_url.
parsed_host.hostname drops the port (e.g., https://example.com:8443 → example.com), causing requests to fail for non-standard ports. Use netloc instead:
parsed_host = urlparse(host)
scheme = parsed_host.scheme or ("https" if use_ssl else "http")
-hostname = parsed_host.hostname or host
+netloc = parsed_host.netloc or host
-self.base_url = f"{scheme}://{hostname}"
+self.base_url = f"{scheme}://{netloc}"Current tests pass hosts without explicit ports, so this bug is latent. However, any caller providing host with a port (e.g., via configuration or discovery) will experience connection failures.
📝 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.
| parsed_host = urlparse(host) | |
| scheme = ( | |
| parsed_host.scheme | |
| if parsed_host.scheme | |
| else ("https" if use_ssl else "http") | |
| ) | |
| hostname = parsed_host.hostname if parsed_host.hostname else host | |
| scheme = parsed_host.scheme or ("https" if use_ssl else "http") | |
| hostname = parsed_host.hostname or host | |
| self.base_url = f"{scheme}://{hostname}" | |
| parsed_host = urlparse(host) | |
| scheme = parsed_host.scheme or ("https" if use_ssl else "http") | |
| netloc = parsed_host.netloc or host | |
| self.base_url = f"{scheme}://{netloc}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@airos/base.py` around lines 60 - 64, The current construction of
self.base_url uses parsed_host.hostname which drops explicit ports; update the
logic in the url parsing block (symbols: urlparse, parsed_host, scheme, use_ssl,
hostname, base_url) to use parsed_host.netloc (falling back to host if netloc is
empty) when composing the authority portion so ports are preserved; keep the
existing scheme selection (parsed_host.scheme or ("https" if use_ssl else
"http")) and then set self.base_url = f"{scheme}://{netloc}" (with netloc
defaulting to host when needed).
1 participant
Fix ruff upgrade #172
Summary by CodeRabbit
Chores
Refactor