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

[Stefan R. <ste...@gm...>]

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...>:

> 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...> <ja...@fa...>
> *An:* "Stefan Richthofer" <Ste...@gm...>
> <Ste...@gm...>
> *Cc:* "Jython Developers" <jyt...@li...>
> <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...> <ja...@fa...>
> *An:* "Jython Developers" <jyt...@li...>
> <jyt...@li...>, "Stefan Richthofer"
> <Ste...@gm...> <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
>
>
>