Richer command output

[germa89]

First PR of the CommandOutput route.

I'm going to structure it in three PRs:

• Non-conflicting implementation of CommandOutput (current)
• Implementation of table like commands
• Implementation of image like commands (Optional)

This PR aims to close #735 by implementing our own class for the command output.

Features:

The output from _run is our custom class.

So far, this custom class has an attribute (_cmd, gateway cmd) which has the name of the command which originated the output. This attribute is transferred if methods such as str.replace(), str.split(), str.splitlines() etc modify the commands output.

Edit

After carefully and long considering the possible options, we are opting for wrapping specific commands instead replacing all command output.

This is faster way to solve this, and a compromise between features and developing time.

[akaszynski]

Rather than subclassing str, subclass UserString. This is a much better solution as our CommandOutput class can be vastly simplified.

from collections import UserString
class MyStr(UserString):                                                                                                                                                                                  
    """Customized subclass of UserString"""

    def duplicate(self):
    """Return a duplicated string"""
        return self + self                                                                                                                                                                                                                                                                               

This works:

>>> mystr = MyStr('woa')
>>> output = mystr.duplicate()
>>> more_output = output.duplicate()
>>> type(more_output)
<class '__main__.MyStr'>

Advantage of this approach is we don’t have to write out all every possible string method into our new custom class.

[germa89]

@akaszynski ... I did thought about it. One big problem is that MyStr is NOT a string, hence:

In [8]: isinstance(more_output, str)
Out[8]: False

Which is a big thing in the current implementation (many string checks across the code, etc). We could change those string checks, but, I think it is better just subclassing str so we don't break the current implementation and introduce an antipattern (?) (expecting text to be a string when it is not).

Hence I had to subclass str, I do agree it is "not elegant" (?). In fact, I did have a look at other ways to implement this, for example this:

# To be deleted after first review of PR.
class CommandOutput2(str):

    ## References:
    # - https://stackoverflow.com/questions/7255655/how-to-subclass-str-in-python
    # - https://docs.python.org/3/library/collections.html#userstring-objects
    # - Source code of UserString

    def __new__(cls, content, cmd=None):
        obj = super().__new__(cls, content)
        obj._cmd = cmd
        return obj

    # def _copyobj(self, seq):
    #     # __new__ needs type and the args.
    #     # return self.__new__(type(self), seq, self._cmd)

    def __getattribute__(self, name):
        if name in dir(str) and name != '_copyobj': # only handle str methods here
            def method(self, *args, **kwargs):
                value = getattr(super(), name)(*args, **kwargs)
                # not every string method returns a str:
                if isinstance(value, str) and not isinstance(value, CommandOutput2):
                    # return type(self)(value)
                    return self.__new__(value, self._cmd)

                elif isinstance(value, list):
                    return [self.__new__(i, self._cmd) for i in value]

                elif isinstance(value, tuple):
                    return tuple(self.__new__(i, self._cmd) for i in value)
                else: # dict, bool, or int or type
                    return value
            return method.__get__(self) # bound method
        # else: # delegate to parent
        #     return super().__getattribute__(name)

*This code is included in the branch currently, and it will be deleted after we agree on the implementation.

This implementation do not have this problem. However, there is a point in the Sphinx documentation where it check the content of one of the special attributes (I think it was __class___, I'm not sure) and that attribute wasn't correctly mapped using this method.

Again, this is not very elegant. But I can't come up with a better solution at the moment. I'm happy to heard suggestions though.

[akaszynski]

Does it have to be a string? Typically Python users ducktype, so I doubt that we would have a usage issue. If it comes to breaking tests, it should be fine for us to change them should we see this feature as worthwhile.

[akaszynski]

Looks like we can inherit from both UserString and Str:

from collections import UserString

class MyStr(UserString, str):
    """Customized subclass of UserString"""

    def duplicate(self):
        return self + self

Both the instance and the output of duplicate are still considered a subclass of str

>>> mystr_instance = MyStr('woa')
>>> out = mystr_instance.duplicate()
>>> outofout = out.duplicate()
>>> isinstance(out, str)
True
>>> isinstance(outofout, str)
True

And inherit from str:

>>> out.startswith('w')
True

This is the way.

[germa89]

Hi @akaszynski

I'm trying your approach and it have coded this:

from collections import UserString

class MyStr(UserString, str):
    """Customized subclass of UserString"""

    # def __new__(cls, content, cmd=None):
    #     obj = super().__new__(cls, content)
    #     obj._cmd = cmd
    #     return obj

    def __init__(self, content, cmd=None):
        super().__init__(content)
        self._cmd = cmd

which gives me:

>>> mystr = MyStr('this is the ouput', '/command')
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-10-ab1e6c9f1c25> in <module>
----> 1 aa = MyStr('asdf', '/inp')

TypeError: decoding str is not supported

I find difficult (probably I'm missing something) to solve that without rewriting all the methods.
I feel like the problem is related with the second argument ('/command') being passed to the str.new() method.
I find the multi-inheritance a bit confusing.

[akaszynski]

Either have cmd or command, but not both:

>>> import this
...
There should be one-- and preferably only one --obvious way to do it.
...

[germa89]

I was thinking in having two methods:

• cmd for the command (/input)
• command for the full command (/input, file,txt,
  They will both reference to the private _cmd.

[akaszynski]

This is looking good, agree with the approach. Seems that occam's razor applies to software as well.

Please provide documentation in our user guide showing how they can access cmd (or command depending on what you choose`) since it won't be obvious that we're returning a monkey batched string.

[germa89]

@akaszynski I realized that not modifying the methods (as I did at the first proposal) makes that the output is not our custom str

from ansys.mapdl.core.commands import CommandOutput

>>> bb = CommandOutput('asdf','inp,test,txt')

>>> bb.command
'inp,test,txt'

>>> bb.cmd
'inp'

>>> type(bb)
ansys.mapdl.core.commands.CommandOutput

>>> bb[1:] + bb[0]
'sdfa'

>>> type(bb[1:] + bb[0])
str

Hence we miss "our" class after the modification. Hence we cannot use to_array or to_dataframe after the original CommandOutput class is modified.

This happens in Mapdl.run, for example:

https://github.com/pyansys/pymapdl/blob/fe45dfc9409021d93563408c9e5096644b77541f/ansys/mapdl/core/mapdl.py#L2139-L2152

Or also could happen if we use decorators.

[germa89]

Opting for wrapping the desired commands.

[akaszynski]

Opting for wrapping the desired commands.

Let's then abandon this PR or reduce the scope by not wrapping the output in _run.

[germa89]

Ok... So we move towards applying wrappers to specific functions.

[germa89]

I would water down this PR then.

We won't wrap the run or _run output. We would wrap only certain commands.

I believe the code here (CommandOutput) is still useful because the output classes (the array-like, data-frame like, etc) will be based on this one.

I would then approve this one after I edit the description.

[akaszynski]

We won't wrap the run or _run output. We would wrap only certain commands.

Exactly, and we do it at the command level, but only for certain commands with a subclass of this class that does something.

[akaszynski]

LGTM.