Re: [Jython-dev] PyShadowString test, ideas, questions

[Jeff A. <ja...@fa...>]

1) I don't recall the thinking at the time. There are an awful lot of 
constructors here and I suspect I wanted to minimise. It looks like the 
ultimate constructor guarantees stringiness, so that wasn't the 
explanation. I'd be fine with public.

2) I think you're right about new. I'd prefer to add the for_type clause 
than prohibit it, because sub-classing is required by the tests (for it 
to be equivalent to str in all respects). We discussed it below, I see.

Please feel free to go ahead with the changes.

Jeff Allen

On 26/08/2018 16:17, Stefan Richthofer wrote:
> Jeff, I encountered two more concerns with PyShadowString recently:
>
> 1) I would like to have the PyObject/PyObject constructor public. See 
> https://github.com/Stewori/JyNI/pull/29. Is there an actual concern to 
> keep it private?
> 2) I noticed that PyShadowStringDerived is never used. In 
> shadowstr_new some equivalent to this code (e.g. from PyString) would 
> be needed:
> if (new_.for_type == subtype) {
>      return new PyString(str);
> } else {
>     return new PyStringDerived(subtype, str);
> }
> If we wouldn't add something like this there is no point to have 
> PyShadowStringDerived at all. We should either add this or remove 
> PyShadowStringDerived and prohibit subclassing of PyShadowString. Or 
> do I overlook something?
> I can perform these changes if it's okay for you.
>
> Best
>
> -Stefan
>
> Am Do., 30. Nov. 2017 um 08:13 Uhr schrieb Jeff Allen 
> <ja...@fa... <mailto:ja...@fa...>>:
>
>     The point of having a copy of the list would be to prevent this:
>
>     >>> s = PyShadowString("hello", "bonjour")
>     >>> s.addtarget(r"org\.python\.util\.InteractiveConsole")
>     >>> t = s[:3]
>     >>> t == "bon"
>     True
>     >>> s. gettargets().pop()
>     ('org\\.python\\.util\\.InteractiveConsole',)
>     >>> t == "bon"
>     False
>
>     It is not a problem if the slice is only transient, as in if
>     sys.platform[:4] == 'java' , which I agree is the only sort of
>     slicing attested in the stdlib. If we imagine a slice getting a
>     life of its own, the behaviour above would be very confusing, but
>     I'm happy to risk that for now.
>
>     I think I have a decent implementation now of these several changes.
>
>     Jeff
>
>     Jeff Allen
>
>     On 27/11/2017 21:31, Stefan Richthofer wrote:
>>     > I wonder why we write if sys.platform[:4] == 'java' anyway.
>>     It's more work than startswith and you have to count to 4 yourself.
>>     It's a matter of fact that external libraries sometimes use
>>     slicing for platform check. So far the implementation served all
>>     use cases I found.
>>     > suggest we expect PyShadowString(a, b)[m,n] to be
>>     PyShadowString(a[m,n], b[m,n]), and I can see advantages to that.
>>     Good spot. Luckily this is mostly used for "win" which is shorter
>>     than "java", but of course your version is safer and more correct.
>>     Please let's apply that.
>>     > Also, if the targets list is mutable, I think each slice must
>>     have its own.
>>     I'm not sure if that is necessary. At least for all use cases I
>>     know it is fine if the slice inherits or shares target list
>>     from/with the original PyShadowString. Do I overlook something?
>>     *Gesendet:* Montag, 27. November 2017 um 21:33 Uhr
>>     *Von:* "Jeff Allen" <ja...@fa...> <mailto:ja...@fa...>
>>     *An:* "Stefan Richthofer" <Ste...@gm...>
>>     <mailto:Ste...@gm...>
>>     *Cc:* "Jython Developers" <jyt...@li...>
>>     <mailto:jyt...@li...>
>>     *Betreff:* Re: PyShadowString test, ideas, questions
>>
>>     Thanks Stefan:
>>
>>     I'm happy with your answers. I have tried isBaseType = true and
>>     can run the str tests now (passes of course). I've added a
>>     *Derived.java file.
>>
>>     7. Slicing is interesting. Nice touch.
>>
>>     >>> s = PyShadowString("hello", "bonjour")
>>     >>> s[:3]
>>     'hel ( ==bon for targets )'
>>
>>     I appreciate the main use case is sys.platform[:n] , but these
>>     were surprising (because the slice is only interpreted relative
>>     to the main string):
>>
>>     >>> s[:7]
>>     'hello ( ==bonjo for targets )'
>>
>>     >>> s[-3:]
>>     'llo ( ==njo for targets )'
>>
>>     I suggest we expect PyShadowString(a, b)[m,n] to be
>>     PyShadowString(a[m,n], b[m,n]), and I can see advantages to that.
>>     Also, if the targets list is mutable, I think each slice must
>>     have its own.
>>
>>     I wonder why we write if sys.platform[:4] == 'java' anyway. It's
>>     more work than startswith and you have to count to 4 yourself.
>>
>>     Jeff
>>
>>     Jeff Allen
>>     On 26/11/2017 10:16, Stefan Richthofer wrote:
>>
>>         Trying to give answers as far as I have them...
>>
>>         1. Should I be able to do this?
>>
>>         >>> s.gettargets().pop()
>>         ('org\\.python\\.util\\.InteractiveConsole',)
>>         >>> s == 'bonjour'
>>         False
>>
>>         I think that's okay. target list is not private to alow
>>         monkeypatching etc in usual Python fashion.
>>
>>         2. This seems like a useful "unconditional", but I wonder if
>>         it is harmful:
>>
>>         >>> s.addtarget(None)
>>         >>> s == 'bonjour'
>>         True
>>
>>         No opinion. This behavior looks okay to me. Whoever uses
>>         PyShadowString must be aware of entering evil zone anyway.
>>
>>         3. Confusing subject, but I'm not sure the exposure of __eq__
>>         is "canonical". I think the convention is __eq__ just wraps
>>         shadowstr___eq__, as done for startswith and __repr__, but
>>         here there is "added value" in __eq__ (also in
>>         PyString.__eq__). shadowstr___eq__ will be exposed as __eq__,
>>         and shouldn't PyShadowString.__eq__ do the same thing? What
>>         we need is complicated by the wrapper PyObject._eq.
>>
>>         I think you're right. The check for isTarget should be moved
>>         to shadowstr___eq__ I suppose. Sorry I overlooked this when I
>>         wrote it.
>>
>>         4. Single-stepping through __eq__ I notice that a call like s
>>         == 'bonjour' (i.e. a call to __eq__) is quite costly, even
>>         when there are no targets that match. The problem is in
>>         isTarget() and so I think I will try changing the logic to:
>>
>>         return other==string || (other==shadow && isTarget())
>>
>>         but also, we can make isTarget() cheaper.
>>
>>         Thats' s a good improvement. Also, isTarget should fail-fast
>>         if target list is empty (I thought I had done it that way).On
>>         the other hand, performance is hardly a priority here,
>>         because platform checks are usually rather seldom, mostly on
>>         startup only.
>>
>>         5. I tried the tests of str on PyShadowString, to prove it is
>>         also a str, but they fail because it cannot be sub-classed.
>>         Is there a reason to prevent sub-classing?
>>
>>         This is because of isBaseType = false? We can remove that. I
>>         just felt that shadow string contains enough magic and should
>>         not be further extended, but there is no concrete reason. We
>>         can allow subclassing.
>>
>>         6. And relatedly, when I was working on PyType I noticed that
>>         it is possible to expose a Java sub-class as the same Python
>>         type as its parent. This would mean that
>>         type(PyShadowString("a", "b")) would be str even though the
>>         behaviour is different. This sounds worth trying.
>>
>>         Hmm, I don't like that. ShadowString is not a string and its
>>         type should be labled accordingly.
>>
>>         *Gesendet:* Sonntag, 26. November 2017 um 09:06 Uhr
>>         *Von:* "Jeff Allen" <ja...@fa...>
>>         <mailto:ja...@fa...>
>>         *An:* "Jython Developers" <jyt...@li...>
>>         <mailto:jyt...@li...>, "Stefan
>>         Richthofer" <Ste...@gm...>
>>         <mailto:Ste...@gm...>
>>         *Betreff:* PyShadowString test, ideas, questions
>>
>>         I've added a test for PyShadowString, our string that can
>>         have two values at once. Trying to design a test raises some
>>         questions.
>>
>>         >>> s = PyShadowString("hello", "bonjour")
>>         >>> s == 'bonjour'
>>         False
>>         >>> s.addtarget(r"org\.python\.util\.InteractiveConsole")
>>         >>> s == 'bonjour'
>>         True
>>
>>         So far so good.
>>
>>         1. Should I be able to do this?
>>
>>         >>> s.gettargets().pop()
>>         ('org\\.python\\.util\\.InteractiveConsole',)
>>         >>> s == 'bonjour'
>>         False
>>
>>         2. This seems like a useful "unconditional", but I wonder if
>>         it is harmful:
>>
>>         >>> s.addtarget(None)
>>         >>> s == 'bonjour'
>>         True
>>
>>         3. Confusing subject, but I'm not sure the exposure of __eq__
>>         is "canonical". I think the convention is __eq__ just wraps
>>         shadowstr___eq__, as done for startswith and __repr__, but
>>         here there is "added value" in __eq__ (also in
>>         PyString.__eq__). shadowstr___eq__ will be exposed as __eq__,
>>         and shouldn't PyShadowString.__eq__ do the same thing? What
>>         we need is complicated by the wrapper PyObject._eq.
>>
>>         4. Single-stepping through __eq__ I notice that a call like s
>>         == 'bonjour' (i.e. a call to __eq__) is quite costly, even
>>         when there are no targets that match. The problem is in
>>         isTarget() and so I think I will try changing the logic to:
>>
>>         return other==string || (other==shadow && isTarget())
>>
>>         but also, we can make isTarget() cheaper.
>>
>>         5. I tried the tests of str on PyShadowString, to prove it is
>>         also a str, but they fail because it cannot be sub-classed.
>>         Is there a reason to prevent sub-classing?
>>
>>         6. And relatedly, when I was working on PyType I noticed that
>>         it is possible to expose a Java sub-class as the same Python
>>         type as its parent. This would mean that
>>         type(PyShadowString("a", "b")) would be str even though the
>>         behaviour is different. This sounds worth trying.
>>
>>         Jeff
>>
>>         -- 
>>         Jeff Allen
>>
>