which
mk
if isinstance(cmd, str):
self.cmd = cmd.replace(r'${ADDR}',ip)
else:
self.cmd = cmd
or
self.cmd = cmd
if isinstance(cmd, str):
self.cmd = cmd.replace(r'${ADDR}',ip)
Steve Holden
opinions are mixed on this. In recent versions of Python you might want
to take the possibility that cmd is Unicode into account by using
id isinstance(cmd, basestring):
regards
Steve
--
Steve Holden +1 571 484 6266 +1 800 494 3119
PyCon is coming! Atlanta, Feb 2010 http://us.pycon.org/
Holden Web LLC http://www.holdenweb.com/
UPCOMING EVENTS: http://holdenweb.eventbrite.com/
Jean-Michel Pichavant
>
> if isinstance(cmd, str):
> self.cmd = cmd.replace(r'${ADDR}',ip)
> else:
> self.cmd = cmd
>
> or
>
> self.cmd = cmd
> if isinstance(cmd, str):
> self.cmd = cmd.replace(r'${ADDR}',ip)
>
>
would'nt fight for it.
What is worrying me the most in your code sample is that self.cmd can
hold diferrent types (str, and something else). That is usually a bad
thing to do (putting None aside).
However, my remark could be totally irrelevant of course, that depends
on the context.
JM
mk
>
> What is worrying me the most in your code sample is that self.cmd can
> hold diferrent types (str, and something else). That is usually a bad
> thing to do (putting None aside).
> However, my remark could be totally irrelevant of course, that depends
> on the context.
That's a valid criticism - but I do not know how to handle this
otherwise really, because the program can be called with "cmd" to run,
or a script to run (or a directory to copy) and in those cases cmd is None.
I guess I could use
if cmd:
self.cmd = ...
But. Suppose that under some circumstances cmd is not string. What then?
I know that isinstance is typically not recommended, but I don't see
better solution here.
Regards,
mk
mk
> cmd= isinstance(cmd,str) and c.replace('${ADDR}',ip) or cmd
Perlish, but I like that. :-)
Regards,
mk
Jean-Michel Pichavant
right you don't have much choice as you are suffering from the program
poor interface.
You can fix this problem by explicitly asking for the thing you want to
do, instead of guessing by inspecting the argument nature.
myProg --help
usage : myProg command [args]
command list:
- cmd: execute the given <arg1> command line
- exec: execute the given script file named <arg1>
- copy: copy <arg1> to <arg2>
example:
>myProg cmd "echo that's cool"
>myProg exec /etc/init.d/myDaemon
>myProg copy /tmp /tmp2
JM
John Posner
(lunatic fringe?)
Last August [1], I offered this alternative:
self.cmd = (cmd.replace(r'${ADDR}',ip)
if isinstance(cmd, str) else
cmd)
But it didn't get much love in this forum!
Peter Otten
Neither.
self.cmd = cmd
self.ip = ip
...
subprocess.call(self.cmd, shell=True, env=dict(ADDR=self.ip))
Please spend a bit more energy on a descriptive subject and an unambiguous
exposition of your problem (in English rather than code) next time.
Peter
mk
> If you can change your program interface, then do it, if not then you're
> right you don't have much choice as you are suffering from the program
> poor interface.
> You can fix this problem by explicitly asking for the thing you want to
> do, instead of guessing by inspecting the argument nature.
>
> myProg --help
>
> usage : myProg command [args]
> command list:
> - cmd: execute the given <arg1> command line
> - exec: execute the given script file named <arg1>
> - copy: copy <arg1> to <arg2>
>
> example:
> >myProg cmd "echo that's cool"
> >myProg exec /etc/init.d/myDaemon
> >myProg copy /tmp /tmp2
>
I sure can change the interface since I'm the author of the entire
program. But I don't see how I can arrange program in a different way:
the program is supposed to be called with -c parameter (command to run),
-s script to run, or -y file_or_dir_to_copy.
Then, I start instances of SSHThread class to do precisely that,
separately for each ip/hostname:
class SSHThread(threading.Thread):
def __init__(self, lock, cmd, ip, username, sshprivkey=None,
passw=None, port=22, script=None, remotedir=None):
threading.Thread.__init__(self)
self.lock = lock
if isinstance(cmd, str):
self.cmd = cmd.replace(r'${ADDR}',ip)
else:
self.cmd = cmd
self.ip = ip
self.username = username
self.sshprivkey = sshprivkey
self.passw = passw
self.port = port
self.conobj = None
self.conerror = ''
self.msgstr = ''
self.confailed = True
if script:
self.setpathinfo(script, remotedir=remotedir)
self.sentbytes = 0
self.finished = False
self.abort = False
It gets called like this:
th = SSHThread(lock, opts.cmd, ip, username=username,
sshprivkey=opts.key, passw=passw, port=port, script=opts.script,
remotedir=opts.remotedir)
..where all the options are parsed by ConfigParser.OptionParser(). So
they are either strings, or Nones.
So in this context this is fine. But I wanted to make the class more
robust. Perhaps I should do smth like this before setting self.cmd?
assert isinstance(cmd, basestring) or cmd is None, "cmd should be string
or None"
and then:
if cmd:
self.cmd = cmd.replace..
?
Entire source code is here:
http://python.domeny.com/cssh.py
regards,
mk
Jean-Michel Pichavant
JM
Bruno Desthuilliers
(snip)
> So in this context this is fine. But I wanted to make the class more
> robust. Perhaps I should do smth like this before setting self.cmd?
>
> assert isinstance(cmd, basestring) or cmd is None, "cmd should be string
> or None"
>
> and then:
>
> if cmd:
> self.cmd = cmd.replace..
And what if cmd happens to be the empty string ?-)
ok, me --->[]
Gerald Britton
> Last August [1], I offered this alternative:
>
> self.cmd = (cmd.replace(r'${ADDR}',ip)
> if isinstance(cmd, str) else
> cmd)
>
> But it didn't get much love in this forum!
I'd probably go for that one as well though I might consider removing
the outer parentheses.
--
Gerald Britton
Bruno Desthuilliers
>
> if isinstance(cmd, str):
> self.cmd = cmd.replace(r'${ADDR}',ip)
> else:
> self.cmd = cmd
What could "cmd" be except a string ? From other posts here, I guess
it's either a string or None ? If yes, then I'd go for this:
if cmd:
cmd = cmd.replace(r'${ADDR}',ip)
self.cmd = cmd
John Posner
Agreed ... except that you *need* the outer parentheses if the statement
occupies multiple source lines.
-John
mk
>> if cmd:
>> self.cmd = cmd.replace..
>
> And what if cmd happens to be the empty string ?-)
>
> ok, me --->[]
Right, I didn't think much when I wrote that. Anyway, that's back to
square one.
I will probably go for this anyway:
assert isinstance(cmd, basestring) or cmd is None, 'cmd has to
be string or None'
if cmd:
cmd = cmd.replace(r'${ADDR}',ip)
self.cmd = cmd
Jean-Michel Pichavant
> robust. Perhaps I should do smth like this before setting self.cmd?
>
> assert isinstance(cmd, basestring) or cmd is None, "cmd should be
> string or None"
>
> and then:
>
> self.cmd = cmd.replace..
>
>
To be honest I have not enough courrage to dive into yout 1000 lines of
script :-)
What I can say however:
1/ your interface is somehow broken. You ask actions through options (-c
-y -s), meaning one can possibly use all these 3 options together. Your
code won't handle it (you are using elif statements). What happens if I
use no option at all ? As the the optparse doc states : if an option is
not optional, then it is not an option (it's a positional argument).
2/ executing a script, or copying a directory are both commands as well.
myProg -s /tmp/myscript.sh
is nothing more than
myProg -c '/bin/sh myscript.sh'
myProg -y file1
is nothing more than
myProg -c 'cp file1 towhatever'
3/ check your user parameters before creating your SSHThread, and create
your SSHThread with already validated parameters. You don't want to
pollute you SSHThread code with irrelevant user error check.
my humble conclusion:
1/ rewrite your interface with
prog command args [options]
2/ Simplify your SSHThread by handling only shell commands
3/ at the CLI level (right after parameter validation), mute you copy &
script
command to a shell command and pass it to SSHThread.
Cheers,
JM
John Posner
> sure, but it will fit nicely on one line if you like
to admonish you not to "top post".
At any rate, I proposed the 3-line format specifically because it
separates the data values from the if-then-else machinery, making it
easier (for me) to read. But there was considerable resistance to
spending so much vertical space in the source code.
-John
Gerald Britton
> Did you mean to take this off-list?
Nope -- just hit the wrong key
Also, I'm contractually obligated to
> admonish you not to "top post".
Contract?
>
> At any rate, I proposed the 3-line format specifically because it separates
> the data values from the if-then-else machinery, making it easier (for me)
> to read. But there was considerable resistance to spending so much vertical
> space in the source code.
Weird! It's three lines and the original was four lines was it not>?
--
Gerald Britton
mk
> To be honest I have not enough courrage to dive into yout 1000 lines of
> script :-)
Understandable.
> What I can say however:
>
> 1/ your interface is somehow broken. You ask actions through options (-c
> -y -s), meaning one can possibly use all these 3 options together. Your
> code won't handle it (you are using elif statements). What happens if I
> use no option at all ?
You get this:
Linux RH [17:35] root ~ # cssh.py -c "ps|wc" -s /tmp/s.sh
Following options that you specified are mutually exclusive:
-s / --script (value: /tmp/s.sh)
-c / --cmd (value: ps|wc)
You have to specify exactly one of options -c / --cmd, or -s / --script,
or -y / --copy.
I wrote additional logic to handle such situations, I don't rely
entirely on optparse.
> 2/ executing a script, or copying a directory are both commands as well.
> myProg -s /tmp/myscript.sh
> is nothing more than
> myProg -c '/bin/sh myscript.sh'
True. But you have to copy the script to remote machine in the first
place. It's more convenient to do this using one option (copy to remote
machine & execute there).
> myProg -y file1
> is nothing more than
> myProg -c 'cp file1 towhatever'
Err but this is command to copy a local file/dir to *remote* machine.
Like scp (in fact it uses scp protocol internally).
mk
>
> > What I can say however:
> >
> > 1/ your interface is somehow broken. You ask actions through options (-c
> > -y -s), meaning one can possibly use all these 3 options together. Your
> > code won't handle it (you are using elif statements). What happens if I
> > use no option at all ?
Arrgh this is my bad day. You get this:
Linux RH [17:35] root ~ # cssh.py -i linux
You have to specify exactly one of the following:
- command to run remotely, using -c command / --cmd command, or
- script to run remotely, using -s scriptfile / --script scriptfile, or
- file/directory to copy, using -y file_or_dir / --copy file_or_dir
John Posner
> Also, I'm contractually obligated to
>> admonish you not to "top post".
>
> Contract?
Joke. (I know it's hard to tell.)
>
>>
>> At any rate, I proposed the 3-line format specifically because it separates
>> the data values from the if-then-else machinery, making it easier (for me)
>> to read. But there was considerable resistance to spending so much vertical
>> space in the source code.
>
> Weird! It's three lines and the original was four lines was it not>?
Yes, but most people (including you, right?) seemed to think that
conditional expressions are best confined to a single line.
I proposed my 3-line alternative for expressions that *cannot*
reasonably be confined to a single line.
-John
Steven D'Aprano
> assert isinstance(cmd, basestring) or cmd is None, "cmd should be string
> or None"
Do not use assertions for input validation. That's not what they're for.
assert is compiled away when you run your code with the -O switch, which
means that the test may never be made at all. You should limit assertions
for testing "this can never happen" situations, verifying pre- and post-
conditions, checking the internal logic of your code, and similar.
See also:
http://nedbatchelder.com/text/assert.html
--
Steven
Steven D'Aprano
Whichever one you like. The differences are insignificance, and
essentially boil down to personal preference.
--
Steven
