search

jarun / ddgr

:duck: DuckDuckGo from the terminal
GNU General Public License v3.0
2.94k stars 139 forks source link

Enable/Disable ANSI Sequences on Windows. #87

Closed guilt closed 5 years ago

guilt commented 5 years ago

Fixes #86

jarun commented 5 years ago

Will port the changes from @zmwangx for googler as discussed in https://github.com/jarun/googler/pull/275. Closing this.

jarun commented 5 years ago

@guilt I have completed porting the changes from googler to ddgr at commit 5016e02564ead9a895eec8fed7f5b8ef73f2fe35. Can you please confirm it works as expected?

guilt commented 5 years ago

Nit: It's a bit wierd when you check for isatty() in two places now (get_colorize) but looks good otherwise. Shouldn't we just use 'colorize' only, assuming it verifies isatty() if 'auto'?

Other than that, it should work as expected.

jarun commented 5 years ago

Shouldn't we just use 'colorize' only, assuming it verifies isatty() if 'auto'

And what happens when user sets opts.colorize = 'always'?

guilt commented 5 years ago

It's set to True, which is what the user wants, don't they? And if it's not a tty, then they probably are saving to a text file which is going to have colored text in it, when viewed with less.

So, technically, the always option is sort of a way to shoot the user in the foot. This can also happen in non-Windows BTW, because this always option is not dependent on Windows.

Anyway, I tried it, and the escape sequences to appear in the text file:

python ddgr --colorize=always ddgr > output.txt

and I'm able to view the colored output.txt using more or less on Linux. So we could remove that check.

jarun commented 5 years ago

And if it's not a tty, then they probably are saving to a text file which is going to have colored text in it, when viewed with less.

I know. But it doesn't look like a very regular use case. From my experience, users of googler/ddgr who deal in text use the json format or plain text (-C). I haven't come across a scenario where one saves color codes to text files to view them in less/more later when (s)he can fetch the results from the web anytime again.

Anyway, given the fact that there is an auto option, I think we can always say - use auto instead of always (which does what it means). I will make the change now.

jarun commented 5 years ago

Wait, why do we need to call set_win_console_mode() when opts.colorize = 'always'?

Does calling the API make any difference when you redirect output?

zmwangx commented 5 years ago

It’s not only pointless to set console mode when stout is not a tty, it’s impossible to set it because guess what, it’s not a console.

jarun commented 5 years ago

Right. The API set_win_console_mode() has nothing to do with redirection to text file.

guilt commented 5 years ago

Well.... what it means is that if colorize is True, there's an error and stdout is not a tty but stderr is, that error will appear pretty ;)

But then colorize=always will not do what it should do, does it? I mean, It's really a nit but I wanted to keep it simple.

jarun commented 5 years ago

I've decided to leave it in its current state and can't spend any more time on trivia.

zmwangx commented 5 years ago

Once again, errors are not colored. Only the prompt is. Also, removing the condition doesn’t color stderr at all, since it will error out on stout.

Coming up with weird fictional use cases doesn’t help with anything. googler is for humans, not fuzzers.

I wrote every step of the logic and most loc in the current iteration of googler, so trust me, I have thought of everything.

guilt commented 5 years ago

It's a simple usecase, ddgr > x.txt , when you are not connected to the Internet or DDG/Google has a problem, you'd emit an unintelligle error to stderr, while stdout may not get anything. That is not a made up usecase.

zmwangx commented 5 years ago 
$ https_proxy=255.255.255.255 googler 'not connected' > /tmp/blah
[ERROR] Failed to connect to proxy server 255.255.255.255: Remote end closed connection without response.

No idea why that's in any way "unintelligle [sic]", and why color/ANSI escape sequence has anything to do with it. I'll say it one last time: ANSI escape sequences are not emitted in any error message.

guilt commented 5 years ago

Looks like I was looking at an older version of code that fed sequences directly to printerr. ddgr looks fine now. This is like a recipe that was used in other similar CLI utilities as well. You could have mentioned that these were reviewed/removed.

Also, hold your horses: there still seems to be some of that code in googler which I think should be fixed, if you want to rid your old code of this:

https://github.com/jarun/googler/blob/fe9de616a90d88e08f14b16aff0e1cfbd006c6e2/googler#L2103