Re: [Pharo-dev] 08/05/17 - Tabular Data Structures for Data Analysis - Oleksandr Zaytsev
[redirected from pharo-gsoc to pharo-dev] On Tue, May 9, 2017 at 7:36 AM, Oleksandr Zaytsev <olk.zaytsev@gmail.com> wrote:
A. Improved my fix of this case: https://pharo.fogbugz.com/f/cases/19396, as suggested by Ben Coman, and created a slice for it (I think it works like PR in Pharo - correct me if I'm wrong).
Thanks Olek. The slice is good, and the additional test is good. Now after loading the slice and running all system tests from World > TestRunner I get 21 errors, since they have code like this... self assert: (Smalltalk globals classNamed: originalName) isNil. which needs to be updated and checked that there is no clash in usage. Anyone know of a method like... self assert: (Smalltalk globals classNamed: originalName) raised: ClassNotFound. which seems more useful than each time doing... | correctErrorRaised | correctErrorRaised := false. [ Smalltalk globals classNamed: #notExistedClass ] on: ClassNotFound do: [correctErrorRaised := true]. self assert: correctErrorRaised. btw, one of the failures is ClassTest>>testMethodsReferencingClass but I'm a little confused by... self assert: (ClassTest methodsReferencingClass: (Smalltalk classNamed: #BehaviorTests)) isEmpty since there is no "BehaviorTests" in the Image, which effectively is... self assert: (ClassTest methodsReferencingClass: nil) isEmpty So is this succeeding by accident? @Pavel, @Vicent can you advise? i guess it should be BehaviorTest not BehaviorTests, which goes to show the value of #classNamed: raising ClassNotFound instead of returning nil. cheers -ben
Hi Ben, On Tue, May 9, 2017 at 9:44 AM, Ben Coman <btc@openinworld.com> wrote:
[redirected from pharo-gsoc to pharo-dev]
On Tue, May 9, 2017 at 7:36 AM, Oleksandr Zaytsev <olk.zaytsev@gmail.com> wrote:
A. Improved my fix of this case: https://pharo.fogbugz.com/f/cases/19396, as suggested by Ben Coman, and created a slice for it (I think it works like PR in Pharo - correct me if I'm wrong).
Thanks Olek. The slice is good, and the additional test is good.
Now after loading the slice and running all system tests from World > TestRunner I get 21 errors, since they have code like this... self assert: (Smalltalk globals classNamed: originalName) isNil. which needs to be updated and checked that there is no clash in usage.
Anyone know of a method like... self assert: (Smalltalk globals classNamed: originalName) raised: ClassNotFound.
Hmmm, classNamed: has always answered nil for nonexistent classes. Did this get changed? Not sure that's right. Why not add checkedClassNamed: or some such?
which seems more useful than each time doing... | correctErrorRaised | correctErrorRaised := false. [ Smalltalk globals classNamed: #notExistedClass ] on: ClassNotFound do: [correctErrorRaised := true]. self assert: correctErrorRaised.
btw, one of the failures is ClassTest>>testMethodsReferencingClass but I'm a little confused by... self assert: (ClassTest methodsReferencingClass: (Smalltalk classNamed: #BehaviorTests)) isEmpty
since there is no "BehaviorTests" in the Image, which effectively is... self assert: (ClassTest methodsReferencingClass: nil) isEmpty So is this succeeding by accident? @Pavel, @Vicent can you advise? i guess it should be BehaviorTest not BehaviorTests, which goes to show the value of #classNamed: raising ClassNotFound instead of returning nil.
cheers -ben
-- _,,,^..^,,,_ best, Eliot
Hi Eliot. 2017-05-09 21:27 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
Hmmm, classNamed: has always answered nil for nonexistent classes. Did this get changed? Not sure that's right. Why not add checkedClassNamed: or some such?
I created this issue. I am sure that returning nil from methods is always bad design and many books describe it. Also it is not consistent to #at: method: Smalltalk globals at: #AbsentClass. "=> raise error" Smalltalk globals classNamed: #AbsentClass "=> nil"
Hi Denis, On Tue, May 9, 2017 at 12:51 PM, Denis Kudriashov <dionisiydk@gmail.com> wrote:
Hi Eliot.
2017-05-09 21:27 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
Hmmm, classNamed: has always answered nil for nonexistent classes. Did this get changed? Not sure that's right. Why not add checkedClassNamed: or some such?
I created this issue. I am sure that returning nil from methods is always bad design and many books describe it. Also it is not consistent to #at: method:
Smalltalk globals at: #AbsentClass. "=> raise error" Smalltalk globals classNamed: #AbsentClass "=> nil"
I get that. But there's lots of code out there that expects classNamed: to answer nil for names tat don't name a class. Why break all that code? If you had added checkedClassNamed: or some thing else then that old code wouldn't be broken. There needs to be some cheap way of checking whether a class with a specific name actually exists. classNamed: fulfilled that need. By redefining it you'e meant that that has to be reimplemented. It's not a good idea to redefine co=re behavior in this way. It breaks lots of code (including VMMaker). -- _,,,^..^,,,_ best, Eliot
2017-05-09 22:16 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
I get that. But there's lots of code out there that expects classNamed: to answer nil for names tat don't name a class. Why break all that code? If you had added checkedClassNamed: or some thing else then that old code wouldn't be broken. There needs to be some cheap way of checking whether a class with a specific name actually exists. classNamed: fulfilled that need. By redefining it you'e meant that that has to be reimplemented. It's not a good idea to redefine co=re behavior in this way. It breaks lots of code (including VMMaker).
You know Pharo is going own way. We want improve bad things. That's why #name will be removed from Object in next Pharo version. That's why Pragma #selector is deprecated. It is hard to do but we want move. In case of #classNamed: users should be fixed. Maybe other people have different opinion?
Hi Denis, On Tue, May 9, 2017 at 2:47 PM, Denis Kudriashov <dionisiydk@gmail.com> wrote:
2017-05-09 22:16 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
I get that. But there's lots of code out there that expects classNamed: to answer nil for names tat don't name a class. Why break all that code? If you had added checkedClassNamed: or some thing else then that old code wouldn't be broken. There needs to be some cheap way of checking whether a class with a specific name actually exists. classNamed: fulfilled that need. By redefining it you'e meant that that has to be reimplemented. It's not a good idea to redefine co=re behavior in this way. It breaks lots of code (including VMMaker).
You know Pharo is going own way. We want improve bad things. That's why #name will be removed from Object in next Pharo version. That's why Pragma #selector is deprecated. It is hard to do but we want move. In case of #classNamed: users should be fixed. Maybe other people have different opinion?
OK. Sio w -- _,,,^..^,,,_ best, Eliot
Hi Denis, On Tue, May 9, 2017 at 2:47 PM, Denis Kudriashov <dionisiydk@gmail.com> wrote:
2017-05-09 22:16 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
I get that. But there's lots of code out there that expects classNamed: to answer nil for names tat don't name a class. Why break all that code? If you had added checkedClassNamed: or some thing else then that old code wouldn't be broken. There needs to be some cheap way of checking whether a class with a specific name actually exists. classNamed: fulfilled that need. By redefining it you'e meant that that has to be reimplemented. It's not a good idea to redefine co=re behavior in this way. It breaks lots of code (including VMMaker).
You know Pharo is going own way. We want improve bad things. That's why #name will be removed from Object in next Pharo version. That's why Pragma #selector is deprecated. It is hard to do but we want move. In case of #classNamed: users should be fixed.
OK, So what's the right way to see if a class exists or not without raising an error?
Maybe other people have different opinion?
_,,,^..^,,,_ best, Eliot
Hi Eliot. 2017-05-10 3:11 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
OK, So what's the right way to see if a class exists or not without raising an error?
We need few methods here (I point to them in issue page): - includesClassNamed: aString - classNamed: aString ifAbsent: aBlock
2017-05-09 18:44 GMT+02:00 Ben Coman <btc@openinworld.com>:
I get 21 errors, since they have code like this... self assert: (Smalltalk globals classNamed: originalName) isNil.
we should replace such places with Smalltalk globals includeClassNamed: originalName
I rewrote that test to self should: [Smalltalk globals classNamed: #notExistedClass] raise: ClassNotFound. It looks much better now. But I can't push it because Smalltalkhub is down. Should I fix these 21 errors (rewrite the tests for example) or did you decide that we don't need ClassNotFound? I'm not an expert in this, but what will happen if some user packages already use 'isNil' to check if a class exists? Oleks -- View this message in context: http://forum.world.st/Re-08-05-17-Tabular-Data-Structures-for-Data-Analysis-... Sent from the Pharo Smalltalk Developers mailing list archive at Nabble.com.
2017-05-09 18:44 GMT+02:00 Ben Coman <btc@openinworld.com>:
[redirected from pharo-gsoc to pharo-dev]
On Tue, May 9, 2017 at 7:36 AM, Oleksandr Zaytsev <olk.zaytsev@gmail.com> wrote:
A. Improved my fix of this case: https://pharo.fogbugz.com/f/cases/19396, as suggested by Ben Coman, and created a slice for it (I think it works like PR in Pharo - correct me if I'm wrong).
Thanks Olek. The slice is good, and the additional test is good.
Now after loading the slice and running all system tests from World > TestRunner I get 21 errors, since they have code like this... self assert: (Smalltalk globals classNamed: originalName) isNil. which needs to be updated and checked that there is no clash in usage.
Anyone know of a method like... self assert: (Smalltalk globals classNamed: originalName) raised: ClassNotFound. which seems more useful than each time doing... | correctErrorRaised | correctErrorRaised := false. [ Smalltalk globals classNamed: #notExistedClass ] on: ClassNotFound do: [correctErrorRaised := true]. self assert: correctErrorRaised.
btw, one of the failures is ClassTest>>testMethodsReferencingClass but I'm a little confused by... self assert: (ClassTest methodsReferencingClass: (Smalltalk classNamed: #BehaviorTests)) isEmpty
since there is no "BehaviorTests" in the Image, which effectively is... self assert: (ClassTest methodsReferencingClass: nil) isEmpty So is this succeeding by accident? @Pavel, @Vicent can you advise? i guess it should be BehaviorTest not BehaviorTests, which goes to show the value of #classNamed: raising ClassNotFound instead of returning nil.
Definitely bug. Case 20039.
cheers -ben
participants (5)
-
Ben Coman -
Denis Kudriashov -
Eliot Miranda -
Oleks -
Pavel Krivanek