Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [recommenders-dev] [models] Simplify configuration of ProjectCoordinateAdvisorService

This discussion is hard to follow without discussing this line by line in code. 

The change generally sounds reasonable to me but I’d like to remind you that breaking APIs is something we shouldn’t take lightly. 
Is the current solution working? Does this change have to go in for next Tuesday?

Marcel

Am 22.11.2013 um 15:33 schrieb Olav Lenz <olav.lenz@xxxxxxxxxxxxxx>:

> Hi Andreas,
> 
> your summaryincludes everything I wanted to mention.
> 
> Best regards
> 
> Olav
> 
> Am 22.11.2013 14:47, schrieb Andreas Sewe:
>> This conversation started in Gerrit [1], but is by now probably more
>> suited for the list:
>> 
>> Olav wrote (in Gerrit):
>>> @Andreas: I thought about and would say: yes and no ;-) The
>>> ProjectCoordinateProvider uses the ProjectCordinateAdvisorService and
>>> add the cache (DependencyInfo --> Pc) functionality. Until here it
>>> sounds like a wrapper. But it also provide the mapping between
>>> workspace artifacts and DependencyInfos. The class has two
>>> responsibilities of that class and I would say that adding the
>>> configuration logic of the ProjectCoordinateAdvisorService would make
>>> it worse.
>>> 
>>> Also I would like to mention an other point: At the moment as user of
>>> the API you can get injected a ProjectCoordinateAdvisorService or/and
>>> a ProjectCoordinateProvider as well. The ProjectCoordinateProvider
>>> access the ProjectCoordinateAdvisorService inclusive the cache logic.
>>> The ProjectCoordinateAdvisorService access the advisors directly. I
>>> think this looks a little bit confusing and inconsistent.
>>> 
>>> I mentioned that because I thought about a solution to fix both: What
>>> do you think about create a wrapper for
>>> ProjectCoordinateAdvisorService which take care of the configuration
>>> AND the cache (DependencyInfo --> Pc) logic? The
>>> ProjectCoordinateProvider will still have the other cache and would
>>> use the wrapped ProjectCoordinateAdvisorService. I think we would get
>>> a better separation and could get rid of the inconsistens by using
>>> ProjectCoordinateAdvisorService without cache and
>>> ProjectCoordinateProvider with cache both.
>> OK, let me rephrase this to make sure we are talking about the same thing:
>> 
>> The IDE-idenpentent ProjectCoordinateAdvisorService is really just list
>> of advisors that can be reconfigured (advisor order can be changed) in a
>> *thread-safe* manner but that doesn't react to any UI events. The
>> Eclipse-specific ProjectCoordinateAdvisorService would then take care of
>> reacting to changes in the users preferences and also maintain a
>> DependencyInfo -> ProjectCoordinate cache.
>> 
>> This makes sense, in particular as UI events (somebody changing their
>> advisors preferences) would case *this* cache to be invalidated.
>> 
>> The second (Eclipse-specific) service would be the
>> ProjectCoordinateProvider. This class provides a second level of caching
>> (workspace elements -> DependencyInfos) as well as a nicer interface
>> (ITypes, etc. instead of DependencyInfos) but would not need to react to
>> changes in the advisor configuration. It would, however, need to react
>> to changes in the workspace.
>> 
>> This sounds very good to me. Or am I missing something?
>> 
>> Andreas
>> 
>> [1] <https://git.eclipse.org/r/#/c/18616/>
> 
> _______________________________________________
> recommenders-dev mailing list
> recommenders-dev@xxxxxxxxxxx
> https://dev.eclipse.org/mailman/listinfo/recommenders-dev



Back to the top