Review Request 121881: Greatly cleanup PluginController::loadPluginInternal.

[Sergey Kalinichev]

> On Jan. 7, 2015, 3:22 p.m., Sergey Kalinichev wrote:
> > Still see no reason why you didn't remove checkForDependencies? IMO it makes things more complicated. You can do the same by using just  loadDependencies.
> > Or at least you could have added a doxygen comment for checkForDependencies, or renamed it to e.g. hasUnresolvedDependencies. Because currently it's not clear what it does, and what returns in each situation.
> 
> Milian Wolff wrote:
>     Renaming it is a good idea indeed, and I'll also extend the comments.
>     
>     The reason it exists is that loadDependencies does just that, i.e. it loads all dependencies. But assume the following is the case:
>     
>     Plugin A depends on B, C, D.
>     
>     Assume D does not exist (not installed) or is explicitly disabled by the user
>     
>     Now, if we'd just use loadDependencies, then B and C would be loaded and then D fails, so A also fails. This is not good, we don't want to load B nor C b/c we know D is going to fail and thus also A. This is what the checkForDependencies step does. Does this make things clearer?

Yes, now I finally get it. Also I think it's worth to be mentioned in the comments then.


- Sergey


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121881/#review73349
-----------------------------------------------------------


On Jan. 6, 2015, 10:36 p.m., Milian Wolff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121881/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2015, 10:36 p.m.)
> 
> 
> Review request for KDevelop and Sergey Kalinichev.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> This hopefully resolves some issues in understanding the flow of
> this method. Some code could also be removed and the warning
> messages better formatted.
> 
> 
> Diffs
> -----
> 
>   shell/plugincontroller.cpp f70fc646e00e257bc0cd0a401cd8106569e23181 
> 
> Diff: https://git.reviewboard.kde.org/r/121881/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Milian Wolff
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20150108/87351d32/attachment.html>