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

Update tonel dependency to Pharo12 branch #1735

Merged
merged 2 commits into from
Sep 15, 2023

Conversation

jecisc
Copy link
Contributor

@jecisc jecisc commented Sep 14, 2023

During the development of Pharo 12 we will bring some changes to tonel such as an improved way to deal with packages and tags and maybe a way to set the deprecated aliases of a class.

This change make Iceberg depend on the branch Pharo12 of the tonel repository.

This branch has one change ahead of the master branch that is the introduction of TonelWriterV3. This new writer will export the package and the tag of classes on top of the category. It also update the reader to be able to read this new format and try to be as backward compatible as possible.

During the development of Pharo 12 we will bring some changes to tonel such as an improved way to deal with packages and tags and maybe a way to set the deprecated aliases of a class. 

This change make Iceberg depend on the branch Pharo12 of the tonel repository. 

This branch has one change ahead of the master branch that is the introduction of TonelWriterV3. This new writer will export the package and the tag of classes on top of the category. 
It also update the reader to be able to read this new format and try to be as backward compatible as possible.
@astares
Copy link
Contributor

astares commented Sep 14, 2023

@jecisc / @tesonep / @guillep / @MarcusDenker / @estebanlm

There is a unified property API for classes and methods in Pharo already to add properties. If I'm not mistaken this was introduced by @MarcusDenker:

Object propertyAt: #alias put: 'OldName'.
(Object propertyAt: #alias) inspect

I remember an old discussion with @estebanlm about Tonel and I think the properties of classes and methods are not serialized in current versions of Tonel.

If there is work now being done on a new TonelWriterV3 maybe this is the way forward to generizalize and serialize the properties (which then could be used for all kinds of different meta annotations).

@MarcusDenker
Copy link
Contributor

Like with methods, where we have properties (not serialized with code) and pragmas (part of code), this should not store all properties.

Instead we should have something similar to a progma, which is rendered as part of the fluid class definition, too

@jecisc
Copy link
Contributor Author

jecisc commented Sep 14, 2023

This first change does not deal with the properties. But the new class deprecation system does. For now it works in initialize methods on the class side. But we want to add it to the FluidBuilder and the TonelV3 format.

It can be an idea indeed to think of a way to extend the class builder and the Tonel format to export those kind of properties. We need to think about it (and right now my priority is to remove the concept of categories as package-tag before the end of my contract :p).

But as Marcus said, it cannot be as easy as exporting the properties since some of them are used for system internals and should not be persisted.

@astares
Copy link
Contributor

astares commented Sep 14, 2023

Maybe we should think about transient vs. persistent properties.

Another thing I noticed in this regard: some slots typically generate the accessor methods dynamically/automagically.

Tonel serialization catches them nonetheless and the generated methods are therefore always marked dirthy in Iceberg although autogenerated/recreated - which is strange and not nice.

So there is no way yet in Pharo to mark methods as "transient" - so they do not need to be stored in git by the Tonel writer.

@jecisc jecisc closed this Sep 14, 2023
@jecisc jecisc reopened this Sep 14, 2023
@jecisc
Copy link
Contributor Author

jecisc commented Sep 14, 2023

@estebanlm This PR now passes, should we integrate?

Copy link
Contributor

@astares astares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Primary change is to use switch from a fixed tagged version "v1.0.19" to a flexible branch "Pharo 12" now. I guess this is done to avoid doing a release of Tonel anytime. From what I tested I was able to load the changes.

image

@jecisc
Copy link
Contributor Author

jecisc commented Sep 14, 2023

@astares yes indeed. We plan other changes to tonel in Pharo 12 and we want to create a new release with the TonelV3 format of the Pharo 12 release at that time. But we still want to integrate the current change in Pharo 12.

In time of the release, the tag will be created

@estebanlm estebanlm merged commit f62268d into pharo-vcs:dev-2.0 Sep 15, 2023
3 checks passed
@jecisc jecisc deleted the depend-on-tonel-Pharo12 branch September 15, 2023 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants