search

houqp / shell.py

shell power for python
MIT License
96 stars 6 forks source link

Pass commands through shell #4

Open tburette opened 10 years ago

tburette commented 10 years ago

First of all nice library. I like the syntax better than plumbum.

I'd like to know if you considered setting shell=True in the Popen consturctor?

It makes the command go through the shell first instead of directly executing the command. It allows to use all the niceties of the shell such as :

I tested it https://github.com/tburette/shell.py/commit/326b1f21fbb8994c80ed51751a9acf07154abe55 and it works but it is a rather large change. For example with the change you can have pip in the declaration of string: 

    ex('ifconfig')
          | 'grep -A 1 eth0 | grep inet' #!!!
          | 'awk "{print $2}"'
          | 'cut -d: -f 2').stdout()

Also one test fails :

line 115, in test_pipe_with_cmd_list
    self.assertEqual(pipeline.stdout(), b'192.168.116.101\n')
AssertionError: '192.168.116.101  Bcast\n' != '192.168.116.101\n'
houqp commented 10 years ago

Hi @tburette ,

Sorry for the late reply.

Yes, I have considered the shell setting before. I decided to give none-shell mode a try first because it looks challenging ;P That said, I was planning to add a mode toggle API so people can turn the shell mode on and off as they wish. I would like to take your PR if you set it to be off by default.

As for the failed test case, that's a bug in current implementation. The $2 is quoted in ", so it should be expanded before passing to awk. I will fix that.

FYI, my next item in todo list is to support env var in none-shell mode. Then maybe work on wildcard a bit.

tburette commented 10 years ago

Ok, I'll give shell a shot. I tried to think of scenarios where it would cause problems such as:

But I couldn't come up with anything fundamentally bad.

I think it'd be best to set the shell option once and have it work for every command of the pipe (instead of an option to set for a single command like my commit above did).

Where do you think would be the best API-wise to set this option?

houqp commented 10 years ago

Ok, I'll give shell a shot. I tried to think of scenarios where it would cause problems such as:

doing bad stuff in the string (such as redirection) compatibility between shell (what if the program is tested on bash but runs on ksh

But I couldn't come up with anything fundamentally bad.

My plan is to support common shell features in both modes:

These features, IMHO, are the good part of shell and should be sufficient for most tasks (let me know if I missed any :P) Users are encouraged to only use these features. Ideally, if they stick only to these features, shell mode and none-shell mode should behaviour exactly the same. If they want to use fancy stuffs that are not supported by all shells, that's fine. They will have to turn on shell mode and they should be prepared to take care of all the possible pitfalls themselves.

BTW, I also consider piping and doing io redirect in string not a good practice, it makes the code harder to read if mixed with shell.py's APIs.

That said, if I failed to find an efficient way to implement wildcard support for none-shell mode, we should just switch to shell mode entirely :P I will work on it once I am done with var expansion.

I think it'd be best to set the shell option once and have it work for every command of the pipe (instead of an option to set for a single command like my

commit above did).

Hm, I think setting option for every single command looks OK to me. What is your consideration here? Performance?

Where do you think would be the best API-wise to set this option?

global option. Not fond of that approach. Setting this option in one part of a program could screw another part. Likewise in multithread scenarios.
optional argument to each command : ex(), p(), ex_all(),...

It would be great if we can keep the API as simple as possible, so people can do "shell" scripting in Python without writing too much boilerplate code.

So I would go for the global option first since it will save more typing for user :) As for multithreading, users should lock the state of option themselves. Also I am not a big fan of doing multithread programming in Python since GIL will probably makes slower, heh.

And in fact we can support both in the future. The none-global approach can produce cleaner code if users only want to set the shell option in one command temporarily.

tburette commented 10 years ago

That said, if I failed to find an efficient way to implement wildcard support for none-shell mode, we should just switch to shell mode entirely :P I will work on it once I am done with var expansion.

Use glob https://docs.python.org/2/library/glob.html

Hm, I think setting option for every single command looks OK to me. What is your consideration here? Performance?

Convenience and ease of use. Setting the option for every pipe would be annoying. Having the option only set for some options could be tricky and a recipe for mistakes.

houqp commented 10 years ago

That said, if I failed to find an efficient way to implement wildcard support for none-shell mode, we should just switch to shell mode entirely :P I will work on it once I am done with var expansion.

Use glob https://docs.python.org/2/library/glob.html

Yeah, I was about to give that a try.

Hm, I think setting option for every single command looks OK to me. What is your consideration here? Performance?

Convenience and ease of use. Setting the option for every pipe would be annoying. Having the option only set for some options could be tricky and a recipe for mistakes.

OK, as you said, let's keep it simple then :)