Hello.
We did recently several cleanups by marking abstract classes as abstract using #isAbstract method (https://github.com/pharo-project/pharo/pull/3087, https://github.com/pharo-ide/Calypso/pull/462) . I would like to discuss here and decide what the right way to implement this method. The logic behind is to only return true when receiver is defining class of this method. And it should be false for any subclasses (if they do not implement own #isAbstract method). There is old pattern for implementation to compare name of class:
It is used in many places (mostly tests). And it was used in recent Pharo PR (3087). We have another pattern in other places which simply compare self with class:
And in Calypso I used "more simplified" version using equality:
I think we would all agree that simplest version is last one. It does not raise any question about why we compare name or why we use identity comparison. So this is my choice in this discussion. Best regards, Denis |
Hi Denis, I'm not too happy with any of those. For my own applications I use the following: isAbstract isReallyAbstract The reason, as stated in the method comment, is that in nearly all of my cases a class with subclasses should be considered abstract. Now, I don't want to go and implement #isAbstract on each of those classes but at the same time I need an escape hatch which is not inherited. I don't like how I have to hack around the method lookup but this works far better from me than the class equality / identity check. On 30 Mar 2019, at 19:35, Denis Kudriashov wrote:
Otherwise, this would be my preferred method: Cheers,
|
In reply to this post by Denis Kudriashov
Le 30/03/2019 à 19:35, Denis Kudriashov a écrit :
> Hello. > > We did recently several cleanups by marking abstract classes as abstract > using #isAbstract method > (https://github.com/pharo-project/pharo/pull/3087, https://github.com/pharo-ide/Calypso/pull/462) > . > > I would like to discuss here and decide what the right way to implement > this method. > > The logic behind is to only return true when receiver is defining class > of this method. And it should be false for any subclasses (if they do > not implement own #isAbstract method). > > There is old pattern for implementation to compare name of class: > > MyAbstractClass class>>isAbstract > ^self name == #MyAbstractClass > > > It is used in many places (mostly tests). And it was used in recent > Pharo PR (3087). > > We have another pattern in other places which simply compare self with > class: > > MyAbstractClass class>>isAbstract > ^self == MyAbstractClass > > > And in Calypso I used "more simplified" version using equality: > > MyAbstractClass class>>isAbstract > ^self = MyAbstractClass > > I think we would all agree that simplest version is last one. It does > not raise any question about why we compare name or why we use identity > comparison. So this is my choice in this discussion. > > Please write arguments about your preferred implementation. And let's > choose single way at the end. There is an idea to add a command into > browser to make classes abstract. And this decision will be used as a > template. > > Personally I always used the last one. (self = MyAbstractClass) At first it is because it's the first implementation I saw. Then later, after seen the other two, I sticked with this one because it seems to be the simpler for me and I like my code to stay the simpler possible if there is no problem with that. Until now I never got a problem with this implementation. > Best regards, > Denis > -- Cyril Ferlicot https://ferlicot.fr signature.asc (836 bytes) Download Attachment |
I also find that the last one resonates with what I would expect - and a real class autocompletes easily too.
I also wonder if a class with subclasses should be considered abstract unless specifically marked otherwise (but we need a simple way of doing this if we can think of one - which isn’t overly clever) Tim Sent from my iPhone > On 30 Mar 2019, at 19:21, Cyril Ferlicot D. <[hidden email]> wrote: > >> Le 30/03/2019 à 19:35, Denis Kudriashov a écrit : >> Hello. >> >> We did recently several cleanups by marking abstract classes as abstract >> using #isAbstract method >> (https://github.com/pharo-project/pharo/pull/3087, https://github.com/pharo-ide/Calypso/pull/462) >> . >> >> I would like to discuss here and decide what the right way to implement >> this method. >> >> The logic behind is to only return true when receiver is defining class >> of this method. And it should be false for any subclasses (if they do >> not implement own #isAbstract method). >> >> There is old pattern for implementation to compare name of class: >> >> MyAbstractClass class>>isAbstract >> ^self name == #MyAbstractClass >> >> >> It is used in many places (mostly tests). And it was used in recent >> Pharo PR (3087). >> >> We have another pattern in other places which simply compare self with >> class: >> >> MyAbstractClass class>>isAbstract >> ^self == MyAbstractClass >> >> >> And in Calypso I used "more simplified" version using equality: >> >> MyAbstractClass class>>isAbstract >> ^self = MyAbstractClass >> >> I think we would all agree that simplest version is last one. It does >> not raise any question about why we compare name or why we use identity >> comparison. So this is my choice in this discussion. >> >> Please write arguments about your preferred implementation. And let's >> choose single way at the end. There is an idea to add a command into >> browser to make classes abstract. And this decision will be used as a >> template. >> >> > > Hi, > > Personally I always used the last one. (self = MyAbstractClass) > > At first it is because it's the first implementation I saw. Then later, > after seen the other two, I sticked with this one because it seems to be > the simpler for me and I like my code to stay the simpler possible if > there is no problem with that. > > Until now I never got a problem with this implementation. > >> Best regards, >> Denis >> > > > -- > Cyril Ferlicot > https://ferlicot.fr > |
In reply to this post by Max Leske
Hi Max. сб, 30 мар. 2019 г. в 19:08, Max Leske <[hidden email]>:
Why it is important? If it is about speed then I don't believe you :) because your approach is very far from being optimized and I guess you are ok with that.
|
In reply to this post by Denis Kudriashov
We can also look into it from the other side. All these methods are definitely a kind of code duplication. I think what we need here is an explicit property for classes. It could be even part of class definition. BTW do we plan new class definition in Pharo 8? сб, 30 мар. 2019 г. в 18:35, Denis Kudriashov <[hidden email]>:
|
In reply to this post by Denis Kudriashov
On 30 Mar 2019, at 20:56, Denis Kudriashov wrote:
> Hi Max. > > сб, 30 мар. 2019 г. в 19:08, Max Leske <[hidden email]>: > >> Hi Denis, >> >> I'm not too happy with any of those. For my own applications I use the >> following: >> >> isAbstract >> ^ self subclasses notEmpty and: [ >> (self class localSelectors includes: #isReallyAbstract) not or: [ >> self isReallyAbstract ] ] >> >> isReallyAbstract >> "Override this method in any class that has children but should >> *not* be considered abstract (while the children may be)." >> ^ true >> >> The reason, as stated in the method comment, is that in nearly all of my >> cases a class with subclasses should be considered abstract. Now, I don't >> want to go and implement #isAbstract on each of those classes but at the >> same time I need an escape hatch *which is not inherited*. >> That's why I work around method lookup by checking for the existence of >> the method first. >> >> I don't like how I have to hack around the method lookup but this works >> far better from me than the class equality / identity check. >> >> On 30 Mar 2019, at 19:35, Denis Kudriashov wrote: >> >> Hello. >> >> We did recently several cleanups by marking abstract classes as abstract >> using #isAbstract method (https://github.com/pharo-project/pharo/pull/3087 >> , >> https://github.com/pharo-ide/Calypso/pull/462) . >> >> I would like to discuss here and decide what the right way to implement >> this method. >> >> The logic behind is to only return true when receiver is defining class of >> this method. And it should be false for any subclasses (if they do not >> implement own #isAbstract method). >> >> There is old pattern for implementation to compare name of class: >> >> MyAbstractClass class>>isAbstract >> ^self name == #MyAbstractClass >> >> >> It is used in many places (mostly tests). And it was used in recent Pharo >> PR (3087). >> >> We have another pattern in other places which simply compare self with >> class: >> >> MyAbstractClass class>>isAbstract >> ^self == MyAbstractClass >> >> Otherwise, this would be my preferred method: >> 1. minimum number of byte codes >> > Why it is important? > If it is about speed then I don't believe you :) because your approach is > very far from being optimized and I guess you are ok with that. I know, and you're right of course. It's just the way I feel about the code :) > >> 2. Simple for refactoring tools to update the binding >> 3. no use of #name >> >> Cheers, >> Max >> >> And in Calypso I used "more simplified" version using equality: >> >> MyAbstractClass class>>isAbstract >> ^self = MyAbstractClass >> >> I think we would all agree that simplest version is last one. It does not >> raise any question about why we compare name or why we use identity >> comparison. So this is my choice in this discussion. >> >> Please write arguments about your preferred implementation. And let's >> choose single way at the end. There is an idea to add a command into >> browser to make classes abstract. And this decision will be used as a >> template. >> >> >> Best regards, >> Denis >> >> |
In reply to this post by Denis Kudriashov
On 30 Mar 2019, at 21:05, Denis Kudriashov wrote:
> We can also look into it from the other side. All these methods are > definitely a kind of code duplication. > I think what we need here is an explicit property for classes. It > could be > even part of class definition. I'm not sure whether that's the right thing to do but I definitely like where you're going. BTW, class initialization via Monticello (class side #initialize) also doesn't use inheritance (although I've always considered that a bug). I.e., Monticello only evalutes #initialize on the class that installed the #initialize method and not on any subclass. Maybe a general mechanism for marking a class as abstract could also include a mechanism to "override" method lookup. Just some thoughts. Max > BTW do we plan new class definition in Pharo 8? > > сб, 30 мар. 2019 г. в 18:35, Denis Kudriashov > <[hidden email]>: > >> Hello. >> >> We did recently several cleanups by marking abstract classes as >> abstract >> using #isAbstract method >> (https://github.com/pharo-project/pharo/pull/3087 >> , https://github.com/pharo-ide/Calypso/pull/462) . >> >> I would like to discuss here and decide what the right way to >> implement >> this method. >> >> The logic behind is to only return true when receiver is defining >> class of >> this method. And it should be false for any subclasses (if they do >> not >> implement own #isAbstract method). >> >> There is old pattern for implementation to compare name of class: >> >> MyAbstractClass class>>isAbstract >> ^self name == #MyAbstractClass >> >> >> It is used in many places (mostly tests). And it was used in recent >> Pharo >> PR (3087). >> >> We have another pattern in other places which simply compare self >> with >> class: >> >> MyAbstractClass class>>isAbstract >> ^self == MyAbstractClass >> >> >> And in Calypso I used "more simplified" version using equality: >> >> MyAbstractClass class>>isAbstract >> ^self = MyAbstractClass >> >> I think we would all agree that simplest version is last one. It does >> not >> raise any question about why we compare name or why we use identity >> comparison. So this is my choice in this discussion. >> >> Please write arguments about your preferred implementation. And let's >> choose single way at the end. There is an idea to add a command into >> browser to make classes abstract. And this decision will be used as a >> template. >> >> >> Best regards, >> Denis >> >> |
In reply to this post by Denis Kudriashov
> On 30 Mar 2019, at 21:05, Denis Kudriashov <[hidden email]> wrote: > > We can also look into it from the other side. All these methods are definitely a kind of code duplication. > I think what we need here is an explicit property for classes. It could be even part of class definition. > BTW do we plan new class definition in Pharo 8? so far not. Just a better infrastructure. I also think that this abstract point is not really exciting. Stef |
In reply to this post by Denis Kudriashov
I use it myself but it always feels a bit strange. On the one hand a class is abstract if it includes something like subclassResponsibility and on the other hand we try to make it some kind of type. If it would be the latter one you could make that a Trait. Feels even more strange but .... Norbert
|
Frankly I found strange that we care about this because
we can still instantiate an abstract class
and I do not really see how annotating that a class is abstract help us. If I can instantiate an object and send messages and the class is abstract but there is no usage of abstract messages who cares? May be I do not have such scenarios. Stef
|
Calypso warns you about missing methods if it doesn’t understand a class is abstract, so it’s useful to avoid those warnings otherwise you become desensitised to them.
Tim
Sent from my iPhone
|
Tim Mackinnon wrote
> Calypso warns you about missing methods if it doesn’t understand a class > is abstract, so it’s useful to avoid those warnings otherwise you become > desensitised to them. > > Tim Or, if the class has subclasses, one could get a suggestion/action to implement the missing method as missingMethod ^self subclassResponsibility Which also has the benefit of working nicely with the "expected (or was that "missing"?) protocol" functionality. Unless you meant something else? Cheers, Henry -- Sent from: http://forum.world.st/Pharo-Smalltalk-Developers-f1294837.html |
This is a use case that is addressed at the level of the method, not of a class.
I see the issue similarly to Stef: as I can utilize a class, it has little meaning to call it abstract. A missing method has a different meaning from the typical meaning associated with a class being abstract. Cheers, Doru > On Apr 1, 2019, at 10:54 AM, Henrik Sperre Johansen <[hidden email]> wrote: > > Tim Mackinnon wrote >> Calypso warns you about missing methods if it doesn’t understand a class >> is abstract, so it’s useful to avoid those warnings otherwise you become >> desensitised to them. >> >> Tim > > Or, if the class has subclasses, one could get a suggestion/action to > implement the missing method as > missingMethod > ^self subclassResponsibility > > Which also has the benefit of working nicely with the "expected (or was that > "missing"?) protocol" functionality. > Unless you meant something else? > > Cheers, > Henry > > > > -- > Sent from: http://forum.world.st/Pharo-Smalltalk-Developers-f1294837.html > -- www.feenk.com "Problem solving should be focused on describing the problem in a way that makes the solution obvious." |
I guess I could have added additional constraints
"and the missing method is called from a method on the "abstract class", and the missing method is defined on subclasses", but I thought that implicit from the context :) To me, an abstract class is one you either can't instatiate (through overriding new), or one containing methods which delegate to methods implemented by subclasses, so it makes no sense to use instances of the class itself. The use case is addressed at the method level, yes, but it *is* for the typical meaning associated with a class being abstract, at least to me. Cheers, Henry Tudor Girba-2 wrote > This is a use case that is addressed at the level of the method, not of a > class. > > I see the issue similarly to Stef: as I can utilize a class, it has little > meaning to call it abstract. A missing method has a different meaning from > the typical meaning associated with a class being abstract. > > Cheers, > Doru > > >> On Apr 1, 2019, at 10:54 AM, Henrik Sperre Johansen < > henrik.s.johansen@ > > wrote: >> >> Tim Mackinnon wrote >>> Calypso warns you about missing methods if it doesn’t understand a class >>> is abstract, so it’s useful to avoid those warnings otherwise you become >>> desensitised to them. >>> >>> Tim >> >> Or, if the class has subclasses, one could get a suggestion/action to >> implement the missing method as >> missingMethod >> ^self subclassResponsibility >> >> Which also has the benefit of working nicely with the "expected (or was >> that >> "missing"?) protocol" functionality. >> Unless you meant something else? >> >> Cheers, >> Henry >> >> >> >> -- >> Sent from: http://forum.world.st/Pharo-Smalltalk-Developers-f1294837.html >> > > -- > www.feenk.com > > "Problem solving should be focused on describing > the problem in a way that makes the solution obvious." -- Sent from: http://forum.world.st/Pharo-Smalltalk-Developers-f1294837.html |
In reply to this post by Henrik Sperre Johansen
On 1 Apr 2019, at 09:54, Henrik Sperre Johansen <[hidden email]> wrote:
I don’t know if we are all talking about the same thing. In my image, I have a class where I haven’t got the isAbstract defined, and so Calypso shows a red message - as it thinks I’m supposed to implement #execute (if it knows my intent that my class is abstract, then it doesn’t show the red warning). I think this is Denis’ question? Sure, if I choose to use the abstract class, then its my choice, but as a lint checker -having useful warnings is helpful. But it is a nuisance defining isAbstract - and I’d almost prefer it assumes its abstract in this case (the normal expectations?) unless I tag it as concrete and signal to users that they might consider using my class (its probably a design smell these days though) |
I ended up being surprised today that classes that have abstract
methods (send #subclassResponsibility) were returning false to `isAbstract` and true to `hasAbstractMethods`. I see that the behavior for isAbstract changed over the years: when it was added [1] it was checking for abstract methods. Then it was changed to always return false and allow overrides [2]. Is there an explicit need to have/keep `isAbstract` at the class level? Users can still instantiate those classes, send message to them and get SubclassResponsibility errors. Tools could implement specific solutions for handling this. Cheers, Andrei [1] https://github.com/pharo-project/pharo/pull/541/files [2] https://github.com/pharo-project/pharo/pull/1090/files On Mon, Apr 1, 2019 at 3:02 PM Tim Mackinnon <[hidden email]> wrote: > > On 1 Apr 2019, at 09:54, Henrik Sperre Johansen <[hidden email]> wrote: > > > Or, if the class has subclasses, one could get a suggestion/action to > implement the missing method as > missingMethod > ^self subclassResponsibility > > Which also has the benefit of working nicely with the "expected (or was that > "missing"?) protocol" functionality. > Unless you meant something else? > > > > I don’t know if we are all talking about the same thing. In my image, I have a class where I haven’t got the isAbstract defined, and so Calypso shows a red message - as it thinks I’m supposed to implement #execute (if it knows my intent that my class is abstract, then it doesn’t show the red warning). I think this is Denis’ question? > > Sure, if I choose to use the abstract class, then its my choice, but as a lint checker -having useful warnings is helpful. But it is a nuisance defining isAbstract - and I’d almost prefer it assumes its abstract in this case (the normal expectations?) unless I tag it as concrete and signal to users that they might consider using my class (its probably a design smell these days though) > |
Hi,
I think that it is good that isAbstract does not mean isInstantiatable. However, I also think that isAbstract is too generic and that it would be better to have the tools implement their meaning rather than relying on a generic term. Cheers, Doru > On Apr 23, 2019, at 5:58 PM, Andrei Chis <[hidden email]> wrote: > > I ended up being surprised today that classes that have abstract > methods (send #subclassResponsibility) were returning false to > `isAbstract` and true to `hasAbstractMethods`. > I see that the behavior for isAbstract changed over the years: when it > was added [1] it was checking for abstract methods. Then it was > changed to always return false and allow overrides [2]. > > Is there an explicit need to have/keep `isAbstract` at the class > level? Users can still instantiate those classes, send message to them > and get SubclassResponsibility errors. Tools could implement specific > solutions for handling this. > > Cheers, > Andrei > > [1] https://github.com/pharo-project/pharo/pull/541/files > [2] https://github.com/pharo-project/pharo/pull/1090/files > > On Mon, Apr 1, 2019 at 3:02 PM Tim Mackinnon <[hidden email]> wrote: >> >> On 1 Apr 2019, at 09:54, Henrik Sperre Johansen <[hidden email]> wrote: >> >> >> Or, if the class has subclasses, one could get a suggestion/action to >> implement the missing method as >> missingMethod >> ^self subclassResponsibility >> >> Which also has the benefit of working nicely with the "expected (or was that >> "missing"?) protocol" functionality. >> Unless you meant something else? >> >> >> >> I don’t know if we are all talking about the same thing. In my image, I have a class where I haven’t got the isAbstract defined, and so Calypso shows a red message - as it thinks I’m supposed to implement #execute (if it knows my intent that my class is abstract, then it doesn’t show the red warning). I think this is Denis’ question? >> >> Sure, if I choose to use the abstract class, then its my choice, but as a lint checker -having useful warnings is helpful. But it is a nuisance defining isAbstract - and I’d almost prefer it assumes its abstract in this case (the normal expectations?) unless I tag it as concrete and signal to users that they might consider using my class (its probably a design smell these days though) >> > -- www.feenk.com "If you interrupt the barber while he is cutting your hair, you will end up with a messy haircut." |
Hi Andrei, Doru, Sorry for the late reply, I've been away on vacations last week :). Denis introduced originally the behaviour making classes return true for #isAbstract only when they had #subclassResponsibility methods in them. Originally, if I'm not mistaken, it was to introduce the `should be implemented` protocol in Calypso: this was originally shown for subclasses of abstract classes that were not abstract themselves (under that definition). My main point against that definition is that sometimes I have hierarchies like: A <|- B <|- C Where conceptually both A and B are abstract, but I only put the subclass responsibility methods in A. However under that definition B was not considered as abstract, though in my design it was. And then I had Calypso asking me all the time to <<implement>> the methods in the superclass, which I did not want to! My point at the time (and that resulted in the split of isAbstract into hasAbstractMethods) was that (at least for me) the heuristic of having abstract methods was super weak. A class is abstract by design and not necessarily because I can compute something from its implementation that tells me that maybe it is abstract. I somehow mark a class as abstract because it is abstract, not the other way around. On Tue, Apr 23, 2019 at 6:10 PM Tudor Girba <[hidden email]> wrote: Hi,
|
On Mon, 29 Apr 2019 at 22:38, Guillermo Polito <[hidden email]> wrote:
Randon idea... ``#Frame abstract`` returning a hypothetical instance of AbstractClassSymbol which double-dispatches to set a property of the class when it is created per... Object subclass: #Frame abstract instanceVariableNames: 'frameNumber frameRolls nextFrame' classVariableNames: '' package: 'Exercism-Bowling' cheers -ben |
Free forum by Nabble | Edit this page |