Skip to content
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

More granular control over HOWDOI_SEARCH_ENGINE #235

Merged
merged 7 commits into from May 15, 2019
Merged

Conversation

rtruxal
Copy link
Contributor

@rtruxal rtruxal commented Apr 15, 2019

added a bing arg to the cmdline interface so HOWDOI_SEARCH_ENGINE can be set within the howdoi process itself.

@WindSoilder
Copy link
Contributor

WindSoilder commented Apr 16, 2019

If we want to add a command line arguments, I would recommend using -e --engine bing rather than simple -b argument.

If we use simple -b argument, what if we need to support another search engine like Duckduckgo or yandex? Finally we will mess up when we support more search engines in the user interface.

@rtruxal
Copy link
Contributor Author

rtruxal commented Apr 16, 2019

Did it.

@paulie-g
Copy link
Contributor

This is going to need some work with the new caching code in the fastcache branch.

@gleitz
Copy link
Owner

gleitz commented Apr 20, 2019

I'm wrestling with adding additional command line arguments that can already be achieved with environment variables. For example consider the following:

alias howdoi_bing='HOWDOI_SEARCH_ENGINE="bing" howdoi'

then

» howdoi -l -n3 format date bash
https://stackoverflow.com/questions/1401482/yyyy-mm-dd-format-date-in-shell-script

================================================================================

https://webcache.googleusercontent.com/search?q=cache:Oar9zh1xk10J:https://stackoverflow.com/questions/1401482/yyyy-mm-dd-format-date-in-shell-script+&cd=1&hl=en&ct=clnk&gl=us

================================================================================

https://stackoverflow.com/questions/6508819/convert-date-formats-in-bash

is different than

» howdoi_bing -l -n3 format date bash
https://stackoverflow.com/questions/1401482/yyyy-mm-dd-format-date-in-shell-script

================================================================================

https://stackoverflow.com/questions/6508819/convert-date-formats-in-bash

================================================================================

https://stackoverflow.com/questions/14152759/linux-bash-date-format

Thoughts?

@paulie-g
Copy link
Contributor

You're absolutely right. With that said, environment variables ought not be the primary interface - that's just poor UX. However, bing results are so poor for howdoi's purposes that I don't think it matters - no one other than people who can't use google for some reason are likely to want to use bing.

@gleitz
Copy link
Owner

gleitz commented Apr 21, 2019

Yes the main reason I could see users switching away from Google is if they're blocked due to a rate limit, or if using howdoi from a country that blocks Google. I think the best thing to do would be a note on the README page that includes the following snippet

alias howdoi_bing='HOWDOI_SEARCH_ENGINE="bing" howdoi'

@paulie-g
Copy link
Contributor

paulie-g commented Apr 21, 2019

I'm not opposed to adding this in future, I just don't currently have the bandwidth to go through what this would do to the new caching. Probably nothing bad other than output cache not hitting, I just don't recall (several ksloc deep into a couple other projects atm).

In principle, I'd like to see a better config layer that accepts a .py config file in appdirs.user_config_dir, env vars, command line arguments or py dict when howdoi is used as a library. And a man page. I'd take that over ad hoc tweaks to particular settings.

Edit: this comment is not me volunteering to implement that ;)

@paulie-g
Copy link
Contributor

To be clear, I see no harm in adding this and no reason not to do it, provided it doesn't break anything. I'd say wait until dev is merged into master, rebase/eliminate merge conflicts if necessary, and wait for @rtruxal to test and confirm that it doesn't break the new caching (by break I mean errors, not misses). Being picky with relatively small patches that are clearly not net negative is not a good way to keep new contributors involved and interested imo.

@gleitz
Copy link
Owner

gleitz commented Apr 25, 2019

Ok I just merged dev into master. Would you please rebase @rtruxal?

README.rst Outdated Show resolved Hide resolved
@paulie-g
Copy link
Contributor

Appears to have no conflicts. @rtruxal please test that it doesn't break caching (run with HOWDOI_DISABLE_CACHE, without, on clear/full cache and so on), and address https://github.com/gleitz/howdoi/pull/235/files#r278707922

If all goes well, and by @gleitz 's assent, I think this can go in :)

@rtruxal
Copy link
Contributor Author

rtruxal commented Apr 26, 2019

@paulie-g - Sounds good. I will try to do this over the weekend.

@gleitz
Copy link
Owner

gleitz commented May 9, 2019

@rtruxal no rush but lemme know if those checks passes and you're ready to merge.

@rtruxal
Copy link
Contributor Author

rtruxal commented May 10, 2019

Gahhh thank you for the reminder.

@rtruxal
Copy link
Contributor Author

rtruxal commented May 13, 2019

So good-news/bad-news:

good news - I checked the new arg against the fastercache branch and everything appears to be fine (using HOWDOI_DISABLE_CACHE, full/empty cache)

bad-news - The optional positional arg doesn't work. howdoi -e do things gets broken up into howdoi ==> -e do ==> things. I can't immediately think of a way to fix this, other than switching it back or making the positional argument mandatory.

@gleitz
Copy link
Owner

gleitz commented May 13, 2019

I think the correct usage should be

howdoi -e bing do things

README.rst Outdated Show resolved Hide resolved
@gleitz gleitz merged commit 12561b0 into gleitz:master May 15, 2019
@gleitz
Copy link
Owner

gleitz commented May 15, 2019

Thanks for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants