fix tempfile and new output in conky module #2273
Conversation
| self.tmpfile = NamedTemporaryFile(prefix="py3status_conky-", suffix=".conf", delete=False) | ||
| self.tmpfile = NamedTemporaryFile( | ||
| prefix="py3status_conky-", suffix=".conf", delete_on_close=False | ||
| ) | ||
| self.tmpfile.write(str.encode(tmp)) | ||
| self.tmpfile.close() | ||
| self.conky_command = f"conky -c {self.tmpfile.name}".split() | ||
|
|
||
| # skip invalid conky errors |
There was a problem hiding this comment.
Please fix this comment too.
There was a problem hiding this comment.
Please fix this comment too.
Could I just remove the comment?
The new var name seems self-explanatory
BTW, thanks for being so swift with the review!
There was a problem hiding this comment.
I think it is fine to keep it as-is... invalid_conky_errors because it is mostly related to outputting conky errors on py3status/i3bar due to invalid conky config, but then conky gets revived... And start displaying unwanted startup output/invalid errors such as invalid setting, later with conky: FOUND... then now with x11 session running.
There was a problem hiding this comment.
They seem like debug outputs, here's a test I did:
$ conky -c test.conf
conky: desktop window (0x433) is root window
conky: drawing to desktop window
conky: FOUND: console
conky: FOUND: ncurses
conky: FOUND: file
conky: FOUND: x11
conky: 'i3' x11 session running 'i3' desktop
^Cconky: received SIGHUP, SIGINT, or SIGTERM to terminate. bye!
Conky doesn't really error out afterwards, I Ctrl-C'd it.
That being said... I'd be ok with either way, I'm just pointing it out.
There was a problem hiding this comment.
Debug/startup outputs aren't supposed to be enabled with console flag on. Seems like the contributors kinda forget or missed that one.
This is really an upstream issue. I am glad we have a solution in place tho.
Anyway, I don't have a preference and this PR is already approved. Thank you for this again. And I see you like this module too (not your first rodeo fixing this). That's great too. 👍😊
There was a problem hiding this comment.
Debug/startup outputs aren't supposed to be enabled with console flag on
Yep, that's what I'd think too.
Maybe they have some option to handle log verbosity? but I've no idea if they intended some other behavior for the "out_to_console": True option.
not your first rodeo fixing this
Yeah, it's been part of my i3 config for some time.
It seems I'm at the frontline for bug finding on new updates, I guess I'll see you on the next Fedora release 😂
This patch includes:
Fixes for 2 problems:
In my case, with conky
1.21.7-pre-(default version with latest Fedora) outputsconky: 'i3' x11 session running 'i3' desktopNamedTemporaryFileneedsdelete_on_close=False(which I thought was implied bydelete=False, but apparantly that is not sufficient)A small refactor:
invalid_conky_errorstoignored_conky_outputsas they're actually just expected outputs that we don't care about (and not really a runtime problems requiring a warning/exit/exception)