thepyedpiper / pyp

The Pyed Piper: Python Power At the Prompt
12 stars 0 forks source link

Bug with `ls | pyp "pp[-1], pp[-2]"` #2

Open krackers opened 1 year ago

krackers commented 1 year ago

The output isn't converted to a string properly, instead we get

(<__main__.HistoryObject object at 0x104acc850>, <__main__.HistoryObject object at 0x104acc8b0>)
krackers commented 1 year ago

Ok I think I fixed it. There's basically two issues first is to remove the wrapping of the tuple in an array in the line makes sure output is in list of lists. It's not clear why this is done, because [pp[-1], pp[-2]] should behave identically to pp[-1], pp[-2] in terms of output visible to the user.

 if type(output) in [HistoryObject]: #makes sure output is in list of lists
            output = [output]

        if type(output) in [str, PypStr, type]: #makes sure output is in list of lists
            output = [[output]]

second is that the addition operator for HistoryOutput needs to be fixed

    def __add__(self,other):  #added for python3 for list comprehensions
        if hasattr(other,'wrapped_object'): #use wrapped value if available, it's not for list comps
            other=other.__wrapped
        return ((self.__wrapped) + other)

    def __radd__(self,other):  #added for python3 for list comprehensions
        if hasattr(other,'wrapped_object'): #use wrapped value if available, it's not for list comps
            other=other.__wrapped
        return (other + (self.__wrapped))

previously it was checking for __wrapped attr via hasattr, which as I understand double-underscored identifiers undergo some fancy python name-mangling thing so it doesn't exist at runtime. Also need to define radd

thepyedpiper commented 1 year ago

great, thanks for identifying this and providing a fix! I'll take a look and may include it in future pyp releases as it seems generally useful.

krackers commented 12 months ago

Another issue with history object is in the definition of __len

Should be

    def __len__(self):
        try:
            length = len(self.__wrapped)
        except : #booleans etc
            length = 1

(using self.__wrapped instead of self.__wrapped_object)

thepyedpiper commented 12 months ago

Ok, awesome, I'll check it out. Can you do me a favor and just attach the full code? If it works well, I'll include it in the next update.

thanks,

T

On Thu, Aug 24, 2023 at 3:18 PM krackers @.***> wrote:

Another issue with history object is in the definition of __len

Should be

def __len__(self):
    try:
        length = len(self.__wrapped)
    except : #booleans etc
        length = 1

(using self.wrapped instead of self.wrapped_object)

— Reply to this email directly, view it on GitHub https://github.com/thepyedpiper/pyp/issues/2#issuecomment-1692487338, or unsubscribe https://github.com/notifications/unsubscribe-auth/AYBHA7ZLZOAS6AWELM4A4DLXW7HLXANCNFSM6AAAAAAWMJYTEA . You are receiving this because you commented.Message ID: @.***>

krackers commented 12 months ago

Sure, I created a gist at https://gist.github.com/krackers/3443b5e175096ba5e790360353a864cc

It contains pyp 3.0.9 as downloaded from pypi, with the fixes above. There are actually 2 versions (the second is in the comments) that differ in how they treat tuples within a powerpipe.

Version 1 (the one in the main gist) treats tuples the same as arrays. So that

echo -ne "a\nb" | pyp "pp[0], pp[1]"

prints out

[0] a
[1] b

(i.e. each element of the tuple becomes its own output row).

The second version (in the comments) treats tuples as a distinct type from arrays, so that echo -ne "a\nb" | pyp "pp[0], pp[1]" prints out

[0) a 1) b]

It's not clear which behavior would be better (and you can always force a certain behavior by using array instead of tuple). To me the former behavior makes more sense both because tuples should behave identical to arrays, and because when you are in powerpipe mode you are operating "column-wise" instead of "row-wise" so the default "concatenation" operation should act column-wise (adding a new row, as opposed to behavior in non-powerpipe mode where , adds a new column).

That said, doing so is technically a non-backwards compatible change since previously a tuple returned in power-pipe mode was kept together.

--

Edit: After thinking about it more, not unrolling tuples actually starts to make more sense (the behavior of the version in the comment of the gist). It allows computing bulk stats on input (e.g. seq 1 10 | pyp "min(pp), max(pp)").

thepyedpiper commented 12 months ago

Great, thank you! I'll take a look.

-Toby

On Thu, Aug 24, 2023 at 7:27 PM krackers @.***> wrote:

Sure, I created a gist at https://gist.github.com/krackers/3443b5e175096ba5e790360353a864cc

It contains pyp 3.0.9 as downloaded from pypi, with the fixes above. There are actually 2 versions (the second is in the comments) that differ in how they treat tuples within a powerpipe.

Version 1 (the one in the main gist) treats tuples the same as arrays. So that

echo -ne "a\nb" | pyp "pp[0], pp[1]"

prints out

[0] a [1] b

(i.e. each element of the tuple becomes its own output row).

The second version (in the comments) treats tuples as a distinct type from arrays, so that echo -ne "a\nb" | pyp "pp[0], pp[1]" prints out

[0) a 1) b]

It's not clear which behavior would be better (and you can always force a certain behavior by using array instead of tuple). To me the former behavior makes more sense both because tuples should behave identical to arrays, and because when you are in powerpipe mode you are operating "column-wise" instead of "row-wise" so the default "concatenation" operation should act column-wise (adding a new row, as opposed to behavior in non-powerpipe mode where , adds a new column).

That said, doing so is technically a non-backwards compatible change since previously a tuple returned in power-pipe mode was kept together.

— Reply to this email directly, view it on GitHub https://github.com/thepyedpiper/pyp/issues/2#issuecomment-1692666447, or unsubscribe https://github.com/notifications/unsubscribe-auth/AYBHA74EFPLRBGEBU23LW4TXXAEPXANCNFSM6AAAAAAWMJYTEA . You are receiving this because you commented.Message ID: @.***>