Re: 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 <[hidden email]> 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

I get 21 errors, since they have code like this...
     self assert: (Smalltalk globals classNamed: originalName) isNil.

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?

Hi Eliot.

2017-05-09 21:27 GMT+02:00 Eliot Miranda <[hidden email]>:

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

2017-05-09 22:16 GMT+02:00 Eliot Miranda <[hidden email]>:

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?  

2017-05-09 22:16 GMT+02:00 Eliot Miranda <[hidden email]>:

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?  

[redirected from pharo-gsoc to pharo-dev]

On Tue, May 9, 2017 at 7:36 AM, Oleksandr Zaytsev <[hidden email]> 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

OK, So what's the right way to see if a class exists or not without raising an error?