-
-
Notifications
You must be signed in to change notification settings - Fork 191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: switch to duckduckgo as default search engine with lynx browser #237
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #237 +/- ##
==========================================
- Coverage 72.50% 72.47% -0.03%
==========================================
Files 60 60
Lines 3880 3887 +7
==========================================
+ Hits 2813 2817 +4
- Misses 1067 1070 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
7b8201e
to
a334986
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to ffd6785 in 55 seconds
More details
- Looked at
113
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. Makefile:30
- Draft comment:
The change fromBROWSER
toPLAYWRIGHT
in the Dockerfile is not reflected in the Makefile. Ensure consistency by updating the Makefile to usePLAYWRIGHT
instead ofBROWSER
. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. gptme/tools/_browser_lynx.py:29
- Draft comment:
The default search engine has been changed fromgoogle
toduckduckgo
. Ensure this change is intentional and update any relevant documentation or comments to reflect this. - Reason this comment was not posted:
Confidence changes required:50%
Thesearch
function defaults to usingduckduckgo
, which is a change from the previous default ofgoogle
. This change should be intentional and documented if it affects functionality.
3. gptme/tools/_browser_lynx.py:50
- Draft comment:
Thetest_search
function has a commented-out line for testing Google search. Consider re-enabling or removing it to ensure comprehensive testing coverage. - Reason this comment was not posted:
Confidence changes required:50%
Thetest_search
function has a commented-out line for testing Google search. This should be addressed to ensure comprehensive testing.
Workflow ID: wflow_uYhkQboTwLUSJvT7
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
# env["LYNX_CFG"] = str(Path("~/.config/lynx/lynx.cfg").expanduser()) | ||
if cookies: | ||
# save them to file to be read by lynx | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The read_url
function has a placeholder for handling cookies, but it is not implemented. Consider implementing the logic to save cookies to a file for Lynx to read, as this is necessary for the intended functionality.
I seem to have lost the source for how one is supposed to set the
CONSENT
cookie :(Switching the default search engine to duckduckgo instead (which had a lite version I was previously unaware of).
Important
Fix setting
CONSENT
cookie for Google search in Lynx and update Docker build to default to Lynx instead of Playwright.search()
in_browser_lynx.py
now attempts to setCONSENT
cookie for Google search.search()
changed to DuckDuckGo.Makefile
andDockerfile.eval
updated to usePLAYWRIGHT=no
by default, installing Lynx instead.test_search()
in_browser_lynx.py
.This description was created by for ffd6785. It will automatically update as commits are pushed.