search

jamesofarrell / i3-swallow

used to swallow a terminal window in i3
MIT License
143 stars 10 forks source link

Overhaul i3-swallow to be more pythonic #1

Closed mrichar1 closed 7 years ago

mrichar1 commented 7 years ago
mrichar1 commented 7 years ago

Hi,

Really like what you've created! I've made some changes to the structure of the code to simplify it a bit and make it more 'pythonic' by removing globals and following the advice of pylint and pep8.

Hope this is of use to you - I won't be offended if you decide not to accept the PR as I have made some big changes! :smiley:

jamesofarrell commented 7 years ago

Hey, this mostly looks good. A well needed clean up.

Looks like moving over to argparse has caused issues with the cmd though. You get this error when passing commands with - or -- in front of its arguments:

[jameso@pluto]: ~/src/i3-swallow>$ i3-swallow feh --scale ~/Downloads/i3.png usage: i3-swallow [-h] [-d] cmd [cmd ...]
i3-swallow: error: unrecognized arguments: --scale /home/jameso/Downloads/i3.png

It seems argparse is treating them as arguments. if you can get that resolved I'll merge the request.

The close was a mistake, sorry.

mrichar1 commented 7 years ago

Hi,

I hadn't considered this to be needed, as I'd tested using the usual posix approach of using the -- args separator - i.e: i3-swallow -- feh --scale /tmp/foo.jpg

However I can see that this might catch people out, so I've switched to using parse_known_args() in argparse, which ignores any extra arguments and just stores them.

There is still one gotcha: i3-swallow feh -d --scale /tmp/foo.jpg

Here i3-swallow will 'steal' the -d and close the window. In this case, the -- separator becomes mandatory.

Let me know what you think!

jamesofarrell commented 7 years ago

Thanks for sorting that out, it looks good. it has been merged.