Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark abstract classes in Microdown package - Second attempt #9211

Closed
astares opened this issue Apr 29, 2021 · 6 comments · Fixed by #9212
Closed

Mark abstract classes in Microdown package - Second attempt #9211

astares opened this issue Apr 29, 2021 · 6 comments · Fixed by #9212

Comments

@astares
Copy link
Member

astares commented Apr 29, 2021

In Issue #9175 and related PR #9176 a change was proposed from my side for the classes

  • MicAbstractAnnotatedBlock
  • MicAbstractBlock
  • MicContinuousMarkedBlock
  • MicListBlock

to return true for #isAbstract (like it is done for many classes in the image) as these classes:

  • are intended as abstract as one can see on parts of the name (by the original authors)
  • or mentioned also as abstract classes in their class comment already from the original authors
  • currently wrongly return false for #isAbstract although they are abstract

Applying the proposed change in PR #9176 would:

  1. make it explicit that they are intended to represent abstract classes (and not hide this fact in class comment like for instance for MicListBlock, MicContinuousMarkedBlock)
  2. show them with the abstract icon in the class pane of calypso
  3. show them as abstract methods in the Roassal UML view (italic as it is in worldwide UML standard)
  4. fix that they return false (although they are abstract) and correctly return true for #isAbstract
  5. bring these Microdown classes into alignment with other #isAbstract implementors for abstract classes

The proposed change was following the regular contribution process in having a review from another person first:
it got reviewed and accepted by @MarcusDenker and was merged then.

Right after that:

SORRY: to be honest from the outside point of view this behavior is neither rational or clear nor in any way reproducible
regarding reasoning. There was neither "a real discussion" as said nor a real reason to revert (or even
violate the integration process with another pair of eyes). One could also call it short-term "mood decision".

Can we please return to rational decisions?

Beside the already mentioned reasons I gave above the following could be said additionally:

  1. Implementing #isAbstract to return true for abstract classes is a common pattern ever since. It might not be ideal, but
    its common practice. It is following what used in many well known and respected internal and external packages from
    SUnit, Roassal, Zinc, ... up to Seaside and many other.

  2. We have 361 implementors of #isAbstract within the Pharo 9 image in the various packages and now Microdown
    package should be an exception and not implement it or continue to wrongly return false although the classes are abstract?

Sorry - but I really fail to see why the change was reverted. I do not understand why @Ducasse brings the image back to a state where these classes wrongly respond to #isAbstract with false although either from name or comment of these classes it says that they are abstract.

So I propose it again and gently ask for more opinions from Pharo team and community side.

@Ducasse
Copy link
Member

Ducasse commented May 1, 2021

I have developed and I still develop Microdown.
And in addition I do not want to get this pattern pushed into Pharo this way.

@Ducasse
Copy link
Member

Ducasse commented May 1, 2021

Your statistics are not really good.
Because the users do different things with them.
Now I would like to know for the complete system what is the added value.
Because we got several problems in the past with the calypso way of "helping" programmers.
So having an abstract class gets often in our way and did not bring much.
So while this can be useful in certain occasions like for tests.
In many cases this could be as in pillar, just another message such as shouldBeConsideredAsExporter for example.
So this is not because we have a little number of use of abstract that we should generalize that.
Sorry but if you do not understand this I cannot do much.
I would like to have a real understanding how the case for using isAbstract (this is not because Roassal displays bad UML
and needs to know if the class is abstract that we should mark all the classes that plays this role as abstract).
Especially I would like to know the cost in terms of maintenance for something so dull.
We are not a statically typed language.
And you see I can instantiate Behavior while people believed that it was abstract.
So how this is possible to have a common concrete superclass that is concrete and subclasses that abstract.
Now if SUnit needs to collect methods from superclass to play them and it needs a way to express it. Why not
but then on Test classes.
But again you can ignore what I'm saying and think that I'm just an idiot.

@Ducasse
Copy link
Member

Ducasse commented May 1, 2021

BTW

Object isAbstract
>>> false

ProtoObject isAbstract
>>> false

So it means that all the tools such calypso cannot use the isAbstract information correctly.
In language supporting abstract (and Pharo is not one of these!!!), an abstract subclass should not inherit from a concrete superclass.

@Ducasse
Copy link
Member

Ducasse commented May 1, 2021

So to me isAbstract is a bad concept. Different users should introduce specific methods that clarifies the intent.
And Pharo should not grow with methods of nearly interest.

@astares
Copy link
Member Author

astares commented May 3, 2021

I have developed and I still develop Microdown.

Yes - which to me looks more to be the reason you now object. Because for other packages it was
just fine as there were similar PRs the same week without you or anyone speaking up. These
have not been rolled back.

And in addition I do not want to get this pattern pushed into Pharo this way.

It already is part of Pharo since a long time. I did not invent it - just use it like it is used by others
in internal and external packages.

Your statistics are not really good.

It's also not my statistics - it is the number of implementors the standard image shows.

Now I would like to know for the complete system what is the added value.

You ask what is the added value of abstract classes? For sure I do not have to tell you
about the concept of abstract classes in nominative type systems and that classes are objects
too and therefore should correctly respond when asked something (like #isAbstract in this case).

The mentioned classes at the moment respond wrongly to their API as they return false
although intended as abstract. This was also defined by the author you mentioned: you stated
it already yourself in the name and some class comments.

If you are now convinced that making a distinction into concrete vs. abstract classes is bad
then shouldnt we remove it then not for the full image instead of just Microdown?

At least to me #isAbstract is communication to the user to know if and how the class could/should be used.
At the moment the ones mentioned respond to #isAbstract wrongly. For sure the way we mark using method
them could be improved. Possibly in the future we do not have to (over)write anytime a method (for instance
by storing the value as a property).

Because we got several problems in the past with the calypso way of "helping" programmers.

The discussion is independent from past problems in tools like Calypso. If I'm not mistaken it is about having
abstract vs. concrete classes and the possibility to ask them if they are abstract or not. It is communication
to the user and can be used in (browser)tools, visualizations, persistence frameworks or wherever.

So having an abstract class gets often in our way and did not bring much.

So why do you (as author of the package) state them then as abstract in some names and comments yourself?

We are not a statically typed language.

I'm not aware that abstract vs. concrete classes are a concept of static typed systems only.

But again you can ignore what I'm saying and think that I'm just an idiot.

I do not ignore what you say - still I'm not convinced why we should do it differently for Microdown. The result
of your rollback is that the mentioned classes respond wrongly that they are not abstract although you yourself
as author state this in the name and class comment.

If the majority agrees with you to get rid of #isAbstract now then this is fine too - but it should then be done
for the full image.

If a classes in some packages respond correctly and classes in other packages (like Microdown) respond wrongly
it would make no sense at all.

@astares
Copy link
Member Author

astares commented May 3, 2021

and think that I'm just an idiot.

Which I do not BTW

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants