Re: 08/05/17 - Tabular Data Structures for Data Analysis - Oleksandr Zaytsev

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: 08/05/17 - Tabular Data Structures for Data Analysis - Oleksandr Zaytsev

Ben Coman
[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

Reply | Threaded
Open this post in threaded view
|

Re: 08/05/17 - Tabular Data Structures for Data Analysis - Oleksandr Zaytsev

Eliot Miranda-2
Hi Ben,

On Tue, May 9, 2017 at 9:44 AM, Ben Coman <[hidden email]> wrote:
[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.

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
Reply | Threaded
Open this post in threaded view
|

Re: 08/05/17 - Tabular Data Structures for Data Analysis - Oleksandr Zaytsev

Denis Kudriashov
In reply to this post by Ben Coman

2017-05-09 18:44 GMT+02:00 Ben Coman <[hidden email]>:
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
Reply | Threaded
Open this post in threaded view
|

Re: 08/05/17 - Tabular Data Structures for Data Analysis - Oleksandr Zaytsev

Denis Kudriashov
In reply to this post by Eliot Miranda-2
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" 
Reply | Threaded
Open this post in threaded view
|

Re: 08/05/17 - Tabular Data Structures for Data Analysis - Oleksandr Zaytsev

Eliot Miranda-2
Hi Denis,

On Tue, May 9, 2017 at 12:51 PM, Denis Kudriashov <[hidden email]> wrote:
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).



--
_,,,^..^,,,_
best, Eliot
Reply | Threaded
Open this post in threaded view
|

Re: 08/05/17 - Tabular Data Structures for Data Analysis - Oleksandr Zaytsev

Denis Kudriashov

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?  
Reply | Threaded
Open this post in threaded view
|

Re: 08/05/17 - Tabular Data Structures for Data Analysis - Oleksandr Zaytsev

Oleksandr Zaitsev
In reply to this post by Ben Coman
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
Reply | Threaded
Open this post in threaded view
|

Re: 08/05/17 - Tabular Data Structures for Data Analysis - Oleksandr Zaytsev

Eliot Miranda-2
In reply to this post by Denis Kudriashov
Hi Denis,

On Tue, May 9, 2017 at 2:47 PM, Denis Kudriashov <[hidden email]> wrote:

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?  

OK.  Sio w 



--
_,,,^..^,,,_
best, Eliot
Reply | Threaded
Open this post in threaded view
|

Re: 08/05/17 - Tabular Data Structures for Data Analysis - Oleksandr Zaytsev

Eliot Miranda-2
In reply to this post by Denis Kudriashov
Hi Denis,

On Tue, May 9, 2017 at 2:47 PM, Denis Kudriashov <[hidden email]> wrote:

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.

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
Reply | Threaded
Open this post in threaded view
|

Re: 08/05/17 - Tabular Data Structures for Data Analysis - Oleksandr Zaytsev

Pavel Krivanek-3
In reply to this post by Ben Coman


2017-05-09 18:44 GMT+02:00 Ben Coman <[hidden email]>:
[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.

Definitely bug. Case 20039.
 

cheers -ben


Reply | Threaded
Open this post in threaded view
|

Re: 08/05/17 - Tabular Data Structures for Data Analysis - Oleksandr Zaytsev

Denis Kudriashov
In reply to this post by Eliot Miranda-2
Hi Eliot.

2017-05-10 3:11 GMT+02:00 Eliot Miranda <[hidden email]>:
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