feat: Add optional URL return to get_flights and fetch_flights_html functions#104
feat: Add optional URL return to get_flights and fetch_flights_html functions#104chaurasiayush wants to merge 1 commit intoAWeirdDev:devfrom
Conversation
Updated the get_flights function to include a return_url parameter, allowing it to optionally return a tuple of flights and URL. Modified fetch_flights_html to return both HTML and the final URL.
dosubot
bot
added
the
size:M
📝 WalkthroughWalkthroughThe pull request adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
implements feature request #33 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fast_flights/fetcher.py (1)
12-105:⚠️ Potential issue | 🟠 MajorOverloads are missing the
integrationparameter.The actual implementation at line 112 accepts
integration: Integration | None = None, but none of the four overloads include this parameter. This means type checkers will reject valid calls like:get_flights(query, integration=my_integration, return_url=True)Either add corresponding overloads that include the
integrationparameter, or document thatintegrationis intentionally excluded from the public typed API.Example fix for one overload pair
`@overload` def get_flights( q: Query, /, *, proxy: str | None = None, + integration: Integration | None = None, return_url: Literal[True], ) -> tuple[MetaList, str]:Repeat for all four overloads.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fast_flights/fetcher.py` around lines 12 - 105, The four get_flights overloads are missing the integration parameter present in the implementation; update each overload signature to include integration: Integration | None = None (placed after proxy: str | None = None and before return_url) so type-checkers accept calls like get_flights(query, integration=my_integration, return_url=True); ensure all four overload variants (str/q variants with return_url True/False) are updated consistently to match the runtime signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fast_flights/fetcher.py`:
- Around line 170-172: The integration branch currently returns only the base
URL constant (URL) which omits query parameters; update the integration path in
the fetch logic so it returns the full request URL (including query string)
along with the fetched HTML — either by changing integration.fetch_html(q) to
return a tuple (html, full_url) or by constructing the URL the same way
bright_data does (see bright_data.build/construct logic) and return that full
URL instead of the bare URL constant; ensure the calling code that handles the
other branch (which expects (html, URL)) still receives the same (html,
full_url) shape.
- Line 141: fetch_flights_html was changed to return (html, url) breaking
callers that expect a single string; restore backward compatibility by adding an
optional parameter (e.g., return_url: bool = False) to fetch_flights_html and
branch the return: when return_url is False return the html string (old
behavior) and when True return the (html, url) tuple, update the function
signature/type hints and docstring to describe the parameter and both return
shapes (or provide typed overloads), and update tests/docs; alternatively, if
you choose to keep the new tuple-only API, add a clear release note/migration
guide calling out fetch_flights_html change so callers can adapt.
---
Outside diff comments:
In `@fast_flights/fetcher.py`:
- Around line 12-105: The four get_flights overloads are missing the integration
parameter present in the implementation; update each overload signature to
include integration: Integration | None = None (placed after proxy: str | None =
None and before return_url) so type-checkers accept calls like
get_flights(query, integration=my_integration, return_url=True); ensure all four
overload variants (str/q variants with return_url True/False) are updated
consistently to match the runtime signature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| integration: Integration | None = None, | ||
| ) -> str: | ||
| """Fetch flights and get the **HTML**. | ||
| ) -> tuple[str, str]: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if fetch_flights_html is exported publicly and used externally
# Check __init__.py exports
echo "=== Checking public exports ==="
cat fast_flights/__init__.py 2>/dev/null || echo "__init__.py not found"
# Search for external usage patterns of fetch_flights_html
echo -e "\n=== Searching for fetch_flights_html usage ==="
rg -n "fetch_flights_html" --type py -C2Repository: AWeirdDev/flights
Length of output: 1515
🏁 Script executed:
#!/bin/bash
# Get the full function signature and implementation
echo "=== Full fetch_flights_html function ==="
sed -n '135,168p' fast_flights/fetcher.py
# Check for docstrings
echo -e "\n=== Checking for docstrings ==="
sed -n '135,155p' fast_flights/fetcher.py | grep -A 20 '"""'
# Search for tests related to fetch_flights_html
echo -e "\n=== Searching for tests ==="
find . -type f -name "*test*.py" -o -name "test_*" | head -20
rg -n "fetch_flights_html" --type py tests/ 2>/dev/null || echo "No tests found"
# Check for any changelog or documentation about this change
echo -e "\n=== Checking changelog/docs ==="
fd -e md -e rst -e txt | xargs grep -l "fetch_flights_html" 2>/dev/null || echo "No docs mention found"Repository: AWeirdDev/flights
Length of output: 1478
Breaking change: fetch_flights_html return type changed from str to tuple[str, str].
The function is publicly exported and now returns a tuple of (html, url) instead of just the HTML string. External callers expecting a string will break:
# Code that worked before will now fail:
html = fetch_flights_html(query) # Now returns tuple, not stringThe return type is documented in the function's docstring, but this breaking change lacks a release note or migration guide. Consider either:
- Adding a
return_urlparameter to maintain backward compatibility, or - Documenting this as a breaking change in release notes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fast_flights/fetcher.py` at line 141, fetch_flights_html was changed to
return (html, url) breaking callers that expect a single string; restore
backward compatibility by adding an optional parameter (e.g., return_url: bool =
False) to fetch_flights_html and branch the return: when return_url is False
return the html string (old behavior) and when True return the (html, url)
tuple, update the function signature/type hints and docstring to describe the
parameter and both return shapes (or provide typed overloads), and update
tests/docs; alternatively, if you choose to keep the new tuple-only API, add a
clear release note/migration guide calling out fetch_flights_html change so
callers can adapt.
| else: | ||
| return integration.fetch_html(q) | ||
| html = integration.fetch_html(q) | ||
| return html, URL |
There was a problem hiding this comment.
Integration branch returns incomplete URL without query parameters.
The direct fetch path (line 168) returns str(res.url) which includes the full URL with query parameters. However, the integration branch returns only the base URL constant ("https://www.google.com/travel/flights"), omitting query parameters entirely.
This inconsistency undermines the debugging/sharing purpose of returning the URL. Users would expect the full request URL, not just the base endpoint.
Suggested fix
else:
html = integration.fetch_html(q)
- return html, URL
+ if isinstance(q, Query):
+ return html, q.url()
+ else:
+ return html, f"{URL}?q={q}"This mirrors how bright_data.py constructs URLs internally (per relevant snippet at fast_flights/integrations/bright_data.py:38-48).
📝 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.
| else: | |
| return integration.fetch_html(q) | |
| html = integration.fetch_html(q) | |
| return html, URL | |
| else: | |
| html = integration.fetch_html(q) | |
| if isinstance(q, Query): | |
| return html, q.url() | |
| else: | |
| return html, f"{URL}?q={q}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fast_flights/fetcher.py` around lines 170 - 172, The integration branch
currently returns only the base URL constant (URL) which omits query parameters;
update the integration path in the fetch logic so it returns the full request
URL (including query string) along with the fetched HTML — either by changing
integration.fetch_html(q) to return a tuple (html, full_url) or by constructing
the URL the same way bright_data does (see bright_data.build/construct logic)
and return that full URL instead of the bare URL constant; ensure the calling
code that handles the other branch (which expects (html, URL)) still receives
the same (html, full_url) shape.
1 participant
Summary
Enhances the
get_flights()andfetch_flights_html()functions to optionally return the final request URL alongside the flight data, enabling better debugging and URL sharing capabilities.Changes
return_urlparameter toget_flights()function (default:Falsefor backward compatibility)fetch_flights_html()to return a tuple of(html, url)instead of justhtmlLiteraltypes for overloaded function signaturesreturn_url=False(default): ReturnsMetaListonlyreturn_url=True: Returnstuple[MetaList, str]with flights and URLBenefits
Example Usage