3
\$\begingroup\$

I want to be able to accept True/False or a list of acceptable exit statuses for a command. This way I can choose whether to check and default to checking exit status 0 unless a list of exit statuses is provided to check against. My first instinct is to do a double reassignment.

def raw(self, ssh_command, check=False):
    # removed ssh calls and processing
    check = [] if not check else check
    check = [0] if check is True else check
    # c.call_result.status -> <encapsulation object>.<named tuple>.<exit status>
    if c.call_result.status not in check:
        raise RuntimeError("Exit status of command {} was {}.".format(ssh_command, c.call_result.status))
    return c.call_result

Is this design simply non-pythonic. Should I refactor to not accept booleans and change check=None to the function with check = [] if check is None?

def raw(self, ssh_command, statuses=None):
    # removed ssh calls and processing
    # This won't work because we'll check against an empty list always even when None is passed.
    statuses = [] if statuses is None else statuses
    if c.call_result.status not in statuses:
        raise RuntimeError("Exit status of command {} was {}.".format(ssh_command, c.call_result.status))
    return c.call_result

EDIT: Here are what I intended the above to be in order

Accepting booleans or list of exit statuses to accept:

def raw(self, ssh_command, check=False):
    # removed ssh calls and processing which instantiate object c
    check = [0] if check is True else check
    if check and c.call_result.status not in check:
        raise RuntimeError("Exit status of command {} was {}.".format(ssh_command, c.call_result.status))

Not accepting booleans only list of statuses to accept (changed param as this is backwards incompatible):

def raw(self, ssh_command, statuses=None):
    # removed ssh calls and processing which instantiate object c
    if statuses and c.call_result.status not in check:
        raise RuntimeError("Exit status of command {} was {}.".format(ssh_command, c.call_result.status))
    return c.call_result
\$\endgroup\$
4
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Peilonrayz
    May 8 '17 at 9:42
  • \$\begingroup\$ I updated not based on feedback, but based on running my proposed alternative and seeing it not work. I didn't answer my own question as I still was curious about the original problem ignoring my thrown together solution (which was untested). The first chunk was tested. \$\endgroup\$
    – Brian
    May 8 '17 at 10:01
  • \$\begingroup\$ It's unclear to me what the intended meaning of False or None is. You say that it should default to allowing status 0, but I'm not convinced that the code works that way. \$\endgroup\$ May 8 '17 at 13:50
  • \$\begingroup\$ The functions don't work as intended, but I'm not supposed to change the questions code. See my answer for the working version. In the first function it's supposed to be an optional check on exit statuses. It's original form was a boolean where True would check that it was 0 and False wouldn't check. Now I wanted to accept 1 or more exit statuses to check, i.e. [0, 33]. I was looking for a way to support the original inputs and my new desired inputs. \$\endgroup\$
    – Brian
    May 9 '17 at 2:47
1
\$\begingroup\$

Of the two I'd go with the second, it doesn't make too much sense to use Booleans and Lists, but Lists and None makes more sense type wise.

The un-Pythonic part of this code, is you assign to check_values twice, which confused me a bit as I thought you were changing check.

I'd also recommend that you try to move away from global variables, c. But otherwise your code is good, :)


If you want to shorten your code a little bit, in the second version you can use or rather than a turnery, this is something that I've noticed to be somewhat popular in JavaScript, but I'm not too sure on how Python folk think of it.

def _raw(self, ssh_command, statuses=None):
    if c.call_result.status not in (statuses or []):
        raise RuntimeError("Exit status of command {} was {}.".format(ssh_command, c.call_result.status))
    return c.call_result
\$\endgroup\$
1
  • \$\begingroup\$ That's a great suggestion, I may use it in the future. I made a mistake in my original question. I don't want to check the empty list in the case that no statuse are passed in. So I think if statuses and output.status not in statuses: is the best solution. \$\endgroup\$
    – Brian
    May 8 '17 at 9:40
0
\$\begingroup\$

Here is what I ended up with when using statuses. I previously accepted booleans to 'check' so in production I will probably have to accept that with a deprecation warning and simply use the above logic with double reassignment.

I included the full method. I omitted the irrelevant parts before because I thought it would be more confusing, but I think having the object instantiation omitted lead to more confusion than clarity.

# defined at top of module:
ssh_out = namedtuple('ssh_output', ['out', 'err', 'status'])
# raw is inside class ApplianceSSH, which subclasses ApplianceConnection, which has
# internal class ApplianceConnection.ActCmd
def raw(self, ssh_command, statuses=None):
    """
    Send an ssh command with the only processing being newline stripping.

    Threaded timeouts still apply to `raw`. If you need absolute individual control use method `call`.

    :param ssh_command: command to send
    :param statuses: list of acceptable exit statuses
    :return: namedtuple ssh_output
    """
    c = self.ActCmd(ssh_command)
    self.timeout_call(c)
    # Grab the unprocessed call_result before it reaches post_cmd_handler
    assert c.call_result
    out = [line.strip('\n') for line in c.call_result.out]
    err = [line.strip('\n') for line in c.call_result.err]
    # c.call_result.status -> <ssh/rest encapsulation object>.<named tuple>.<ssh exit status>
    output = ssh_out(out=out, err=err, status=c.call_result.status)

    if statuses and output.status not in statuses:
        raise RuntimeError("Exit status of command {} was {}; only {} accepted.".format(
            ssh_command, c.call_result.status, ','.join(str(s) for s in statuses))
        )

    return output
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.