moosetechnology / Famix

An abstract representation of source code. Famix is generic and can describe applications in multiple programming languages.
MIT License
12 stars 22 forks source link

simplify MooseObject>>#attributeAt: / #cacheAt: / propertyAt: #787

Open NicolasAnquetil opened 2 weeks ago

NicolasAnquetil commented 2 weeks ago

Why do we have 3 different notions: Attribute, cache and property ?

Property is using cache which is a dictionary Attribute is a collection

Very confusing

jecisc commented 2 weeks ago

An attribute is an info that cannot be computed again. So when we flush caches, this info should not be removed. So we need to keep this one (there is an explanation in the class comment)

Now I need to check if it makes sense to keep #cache and #property

jecisc commented 2 weeks ago

Properties seems to use the cache but if the value is not present it will check if the entity has an property of the name we gave and if it is the case it will execute the method that has the implementing selector of the property. And most of thus also use a cache! So we have multiple time the same value in multiple caches.

I propose to deprecate this mechanism (Or at least try because maybe I missed something making it interesting and I'll discover it while deprecating it)

In that case do we keep #lookupPropertyNamed:computedAs: since it just call #cacheAt:ifAbsentPut:? Or do I deprecated it as well?

badetitou commented 2 weeks ago

cacheAt is also great when you do want to store things in an entity in the middle of some computation but that is not part of the model (maybe it will be later). I know we use cacheAt:ifAbsent: sometimes. So, at least deprecation.

jecisc commented 2 weeks ago

The #cache API will stay :) We have property API and cache API and the idea would be to merge both because they use the same collection behind and the mechanism added by properties seems useless in our current implementation

NicolasAnquetil commented 1 week ago

cacheAt is also great when you do want to store things in an entity in the middle of some computation but that is not part of the model (maybe it will be later). I know we use cacheAt:ifAbsent: sometimes. So, at least deprecation.

Benoit, I think what you should use in this case is #attributeAt:

I think this clearly shows that their is a confusion that needs to be clarified.

NicolasAnquetil commented 1 week ago

An attribute is an info that cannot be computed again. So when we flush caches, this info should not be removed. So we need to keep this one (there is an explanation in the class comment)

Does anybody uses clear cache ? I know I have used it sometimes when debugging. But very very seldom.

I would really like that we do an effort to simplify the API and clean the memory. Large API means confusion for the users, more things to maintain...

We are a small team and we have a need to constantly innovate and propose new functionalities. To be manageable on the long term, this requires that we keep as simple and clean as possible.

I would like that you seriously think how we can merge the 3 API into only 1 (and only one dictionary).

My proposal is:

(note: if somebody prefer to keep attribute instead of property I don't mind. I would prefer not keeping cache because it has a more restricted meaning)

jecisc commented 1 week ago

I am using the clearing of caches.

It is useful in some situations for example:

For simplification I would personally like to have:

Now, properties is something flushable so we would need to 1) Deprecate the properties API in Moose 11 2) Rename the Attribute API to Properties in Moose 12 and deprecate the Attribute API