Re: PyShadowString test, ideas, questions
Jeff Allen <[email protected]>
| Newsgroups | gmane.comp.lang.jython.devel |
|---|---|
| Message-ID | <[email protected]> |
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
> <[email protected] <mailto:[email protected]>>:
>
> 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" <[email protected]> <mailto:[email protected]>
>> *An:* "Stefan Richthofer" <[email protected]>
>> <mailto:[email protected]>
>> *Cc:* "Jython Developers" <[email protected]>
>> <mailto:[email protected]>
>> *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" <[email protected]>
>> <mailto:[email protected]>
>> *An:* "Jython Developers" <[email protected]>
>> <mailto:[email protected]>, "Stefan
>> Richthofer" <[email protected]>
>> <mailto:[email protected]>
>> *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
>>
>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Jython-dev mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/jython-dev