-
Notifications
You must be signed in to change notification settings - Fork 214
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 #77 - Stop throwing exceptions if $WINDOWID has not been set. #132
Conversation
1 similar comment
Travis builds for python 3.3 and 3.4 are failing but the failures seem to be unrelated with the code changes. |
window_id = int(check_output(['xprop', '-root', '\t$0', | ||
'_NET_ACTIVE_WINDOW']).split()[1], 16) | ||
return int(environ['WINDOWID']) == window_id | ||
xprop_cmd = shlex.split("xprop -root _NET_ACTIVE_WINDOW") |
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.
Any particular reason for using shlex and splitting a str command instead of defining it as a list to begin with?
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.
shlex.split("xprop -root _NET_ACTIVE_WINDOW")
# vs
['xprop', '-root', '\t$0', '_NET_ACTIVE_WINDOW']
IMHO, shlex has the following advantages:
- It is more readable: less visual clutter (quotes and commas), especially if you have a long command.
- It is more easily testable: just copy paste the command and run it on your shell.
All in all I believe that using shlex is a better practice in general and I use it on my own code, but this is not my repo, so if you prefer a list, a list it shall be :)
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.
Makes sense to me 👍
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.
Switch "
use to '
and I'm happy with this change (I should probably make the linter enforce that rule 😁)
1 similar comment
I rebased. BTW, I think that |
The
KeyError
comes from the usage ofenviron["WINDOWID"]
. What should be used instead is theget
method, e.g.environ.get("WINDOWID")
which returnNone
if the key is missing. Of courseNone
is also a non-valid argument forint()
but I think that defaulting to0
is reasonable.