search

eclipse-ocl / org.eclipse.ocl

Eclipse Public License 2.0
0 stars 0 forks source link

[language] Tuning the API and Impl introduced by Bug 251621 #629

Closed eclipse-ocl-bot closed 1 month ago

eclipse-ocl-bot commented 1 month ago

| --- | --- | | Bugzilla Link | 333032 | | Status | CLOSED FIXED | | Importance | P3 normal | | Reported | Dec 21, 2010 12:52 EDT | | Modified | May 27, 2011 03:13 EDT | | Version | 3.0.0 | | Reporter | Adolfo Sanchez-Barbudo Herrera |

Description

After reviewing the current committed code of the Bug 251621 here you have my 0.02 cents ;)

Patch coming soon.

Best regards,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Dec 21, 2010 13:04

Created attachment 185654 (attachment deleted)\ Fixing patch

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 22, 2010 03:47

Adolfo,

agreed on the Environment argument in EnvironmentFactory.createOCLAnalyzer.

The omission of the registry argument for the DefaultOppositeEndFinder may cause problems if clients use specific registries with, e.g., a specific different subset of Ecore models compared to the default registry which in an OSGi environment would be populated based on the Ecore package extension point. How about adding a WeakHashMap-based cache in DefaultOppositeEndFinder such that as long as the Registry is strongly referenced then so will be the DefaultOppositeEndFinder constructed specifically for that registry, and hence calls to a new getInstance(Registry) will return that cached instance? Let me know, and I can produce a patch that contains the extended DefaultOppositeEndFinder with a getInstance(Registry) and the cache.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Dec 22, 2010 05:22

The omission of the registry argument for the DefaultOppositeEndFinder may cause problems if clients use specific registries with, e.g., a specific different subset of Ecore models compared to the default registry which in an OSGi environment would be populated based on the Ecore package extension point. How about adding a WeakHashMap-based cache in DefaultOppositeEndFinder such that as long as the Registry is strongly referenced then so will be the DefaultOppositeEndFinder constructed specifically for that registry, and hence calls to a new getInstance(Registry) will return that cached instance? Let me know, and I can produce a patch that contains the extended DefaultOppositeEndFinder with a getInstance(Registry) and the cache.

Axel,

Sorry, I assumed that the EcoreEnvironment was creating the oppositeEndFinder using the Global Registry in the same fashion as the EvaluationEnvironment does.... I don't know where I was looking at when changing that line of code, which is obviously wrong ;P

Concerning your suggestion, and AFAIU the oppositeEndFinder which is tight to a package registry, your suggestion absolutely makes sense... Besides, it looks like this defaultOppositeFinde is not intended to be extended, is it? If it is the case, ensure you use the final class qualifier and make the constructor private so that clients only use this service static method to obtain the corresponding oppositeEndFinder from a registry.

Best regards,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 22, 2010 07:32

(In reply to comment #3)\ I'll raise another Bugzilla for the DefaultOppositeEndFinder issue.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 22, 2010 07:52

(In reply to comment #4)\ Raised https://bugs.eclipse.org/bugs/show_bug.cgi?id=333082 and added a patch there (https://bugs.eclipse.org/bugs/attachment.cgi?id=185702).

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Dec 22, 2010 08:19

Created attachment 185704 Fixing patch - Try2

Patch which takes into account the https://bugs.eclipse.org/bugs/attachment.cgi?id=185702 contribution.

Best Regards, Adolfo.

:notepad_spiral: ocl333032_TuningBug2561621ContributionUpTry2.txt

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 22, 2010 08:32

(In reply to comment #6)\ Applies fine, tests all green. Can you commit https://bugs.eclipse.org/bugs/attachment.cgi?id=185702 and then your patch?

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Dec 22, 2010 08:47

I still require another comitter's +1 for my own patch... So let's wait for the approval and I'll commit both.

Cheers,\ Adolfo

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 22, 2010 14:31

+1 to removing the redundant Environment argument in createOCLAnalyzer.

-1 to deprecating the default constructor.

Deprecation is a long-winded process. To actually remove an API requires notification to cross-project-dev and then two releases to elapse.

I would prefer to gather a significant number of deprecations to realise and notify just before 4.0 in the hope that they might get eliminated in 5.0. For now I see this as just a gratuitous change; there is no need for users to revise their code. I actually did the change and aborted it since as you found it requires 4 test classes to change.

Also removal of a default constructor is a significant API change, since certain newInstance facilities apply only to the default constructor.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Dec 23, 2010 07:16

Ed,

I've tried to look for our API retention policy, wihtout any luck. As I read in the portal's simultaneous release tracker:

"Projects should define and document their retention policy. This should include both zip distributions and repositories.".... I'm not sure if this should also include the API retention policy...

In the following guidelines:

http://wiki.eclipse.org/Eclipse/API_Central/Deprecation_Policy#Announcing_API_Removal

Explains that only the API to be removed needs to be announced... and again as far as I understand the @deprecation.... "Ey, don't use this public API there is a better way to do what you intend to"..... besides.... "Be careful, this public API method could be removed in the future, so please use the recommended way and don't get surprised if your code doesn't work in future releases"

Anyway, I'm not discussing when we should deprecate. If you prefer to wait, I'll remove the deprecation...

Also removal of a default constructor is a significant API change, since certain newInstance facilities apply only to the default constructor.

I hope you don't mean that we are breaking API while changing our test cases....

If you mean that we should allow default construction "just in case" somebody would like to use newInstance on the EcoreEvaluantionEnvironment, I simply don't agree... We should teach our clients how to use our API, and we simply don't want to allow them use default constructors because they we want to make our clients construct said enviroments using the correspondent factory.....deprecation is the first step (whenever you prefer to do it).... then..... if (and when) appropriate removal..... Again, take into account that deprecation doesn't need to imply a removal

Oh !! Perhaps, following your comment I should open a bugzilla to add the default constructor to the UmlEvaluationEnvironment counterpart... firstly to make it aligned with the Ecore implementation.... secondly who knows if somebody liked to create the said Environment via newInstance facilities... ;)

Following this idea "how we should use our public API", I still believe that we should add the modifications on the test cases so that the creation of the EvaluationEnvironment is made passing the own factory to them... if we want to teach our clients, we firstly need to do the things in the right way...

Best Regards,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 23, 2010 07:38

(In reply to comment #10)\ Adolfo's reasoning sounds right to me. Adding @deprecation is a useful hint. Adjusting tests doesn't break anything and documents intended ways of using the API. @deprecate doesn't imply a point in time when removal happens. That discussion could be had separately.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 23, 2010 12:15

(In reply to comment #10)

Anyway, I'm not discussing when we should deprecate. If you prefer to wait, I'll remove the deprecation...

Of course not, but if this change is a nuisance 4 times for us, then that suggests how often it may also be a nuisance for clients.

Yes we should teach our clients to the API correctly, but I expect to see significant evolution of API for 4.0, so I would prefer to provide one well-considered set of changes for 4.0, rather than incremental changes in 3.1.

If you feel that the use is bad then ok let's deprecate. I just feel that it's change for changes' sake and that we will probably change again before 4.0, so why give our clients this minor hassle.

Our current API is all over the place. For the most part Ecore can do things automatically so defaults are appropriate. This didn't work for UML that needs an explicit environment to cache the UML counterpart of the Ecore, so UML cannot be defaulted. This is irregular and awkward. I hope that the pivot model will again do things automatically.

+1 to removing the redundant Environment argument in createOCLAnalyzer.

+0 to deprecating the default constructor.

We don't have a project-specific API retention policy, so I think we use the default that you referenced. It seems very reasonable.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Dec 23, 2010 13:04

(In reply to comment #12)

(In reply to comment #10)

Anyway, I'm not discussing when we should deprecate. If you prefer to wait, I'll remove the deprecation...

Of course not, but if this change is a nuisance 4 times for us, then that suggests how often it may also be a nuisance for clients.

Nuisance that make things more consistent should be welcomed (see below).

Yes we should teach our clients to the API correctly, but I expect to see significant evolution of API for 4.0, so I would prefer to provide one well-considered set of changes for 4.0, rather than incremental changes in 3.1.

If you feel that the use is bad then ok let's deprecate. I just feel that it's change for changes' sake and that we will probably change again before 4.0, so why give our clients this minor hassle.

Kk, let's try to explain my feelings

As you noticed in the Axel's contribution's bug (Bug 251621 commnt 107 ), The EnvironmentFactory is detected as a good centralized place to create instances of derived classes such us Environments, EvaluationEnvironments, OppositeEndFinders, etc

If clients defined their derived EnvironmentFactory, for instance to use (create) customized OppositeEndFinder for their Environments and EvaluationEnvironments and said clients used the default EcoreEvaluationEnvironment constructor instead of using the constructor which receives the clients EnvironmentFactory, then the EcoreEvaluationEnvironment would be using the defaultOppositeEndFinder (because the evaluation environment has not received the factory which provides de customized OppositeEndFinder) when the client would have expected to use its own customized OppositeEndFinder...

An example of these clients could have been our own test cases which creates customized EnvironmentFactories which creates default constructed EvaluationEnvironments, excepting they are not requiring any customized OppositeEndFinder. But if it were case, the nuisance would have helped us to fix the problem.

I hope that example is understood to be able to obtain your +1.

Our current API is all over the place. For the most part Ecore can do things automatically so defaults are appropriate. This didn't work for UML that needs an explicit environment to cache the UML counterpart of the Ecore, so UML cannot be defaulted. This is irregular and awkward. I hope that the pivot model will again do things automatically.

+1 to removing the redundant Environment argument in createOCLAnalyzer.

+0 to deprecating the default constructor.

I'm afraid this doesn't help. Since the only +1 received in all the patch comes from a non-committer, I can't currently take a quick action. Several options here, in my preferred ordered:

a) waiting for you, alex, laurent's +1.\ b) waiting for official Axel's committer status.\ c) partially committing the patch, and create a new bugzilla with the residual patch (which would rely on a) and b), BTW)

We don't have a project-specific API retention policy, so I think we use the default that you referenced. It seems very reasonable.

Ok, I think that I specify this in the simultaneous release tracker. Anyway, I think that we still need to establish some kind of Retention Policy concerning downloadable zips and P2 repositories.... I'll probably create a wiki page in our wiki.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 23, 2010 13:39

(In reply to comment #13)

b) waiting for official Axel's committer status.

If it helps, this is what I received a few hours ago:

"Dear Axel Uhl,\ Your committer election for modeling.mdt.ocl has been completed and\ approved by the PMC. Our records indicate from your email address that you\ are a new committer. Please follow the instructions in item 5 "Paperwork"\ of:..."

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Dec 24, 2010 05:19

(In reply to comment #14)

(In reply to comment #13)

b) waiting for official Axel's committer status.

If it helps, this is what I received a few hours ago:

"Dear Axel Uhl, Your committer election for modeling.mdt.ocl has been completed and approved by the PMC. Our records indicate from your email address that you are a new committer. Please follow the instructions in item 5 "Paperwork" of:..."

Great !!!

Committed to HEAD, It will be available in M5.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Dec 24, 2010 05:20

Resolving as fixed.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 24, 2010 05:58

(In reply to comment #16)

Resolving as fixed.

What happened to the removal of the redundant Environment argument in createOCLAnalyzer? From what I see, it's still in the CVS HEAD copy of EcoreEnvironmentFactory. Ed had +1ed it. Won't you change that too?

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Dec 24, 2010 07:09

(In reply to comment #16)

Resolving as fixed.

What happened to the removal of the redundant Environment argument in createOCLAnalyzer? From what I see, it's still in the CVS HEAD copy of EcoreEnvironmentFactory. Ed had +1ed it. Won't you change that too?

Could you recheck, please?

EcoreEnvironmentFactory v1.4 at CVS really has the expected change at the line 197.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 24, 2010 07:19

(In reply to comment #18)\ Sorry, must have been some problem with my local git cvsimport synchronization. I re-synched my git, it got another few updated from CVS that previously didn't get updated, now everything builds just fine.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on May 27, 2011 03:13

Closing