Re: [linuxtools-dev] Change in linuxtools/org.eclipse.linuxtools[master]

[Aleksandar Kurtakov <akurtako@xxxxxxxxxx>]

This commit really makes me sad.
Copy/pasting code is not the way to do stuff. 
* We have shared bundles where implementation for such components can be put
* we have class inheritance so a base class containing the common functionality with children for local and remote stuff are still an option
* we have class names that are supposed to be meaningful (ProviderLaunchConfigurationDelegate extends ProfileLaunchConfigurationDelegate ???)
* why on earth someone has to even copy constants class and redefine even the PLUGIN_ID ?

I do welcome the work on remote tools but not at the price of screwing the codebase. 
Please revert this commit and try to come with one that does things in a clean way. I would better see some functionality being delayed with one release than having to do the same fix in N places.

Alexander Kurtakov
Red Hat Eclipse team

----- Original Message -----
> From: "Otavio Pontes (Code Review)" <gerrit@xxxxxxxxxxx>
> To: "Otavio Pontes" <obusatto@xxxxxxxxxxxxxxxxxx>, "Daniel Henrique Barboza" <danielhb@xxxxxxxxxx>
> Sent: Friday, December 14, 2012 2:55:58 PM
> Subject: Change in linuxtools/org.eclipse.linuxtools[master]: First version of remote unified profile launcher
> framework.
> 
> Otavio Pontes has submitted this change and it was merged.
> 
> Change subject: First version of remote unified profile launcher
> framework.
> ......................................................................
> 
> 
> First version of remote unified profile launcher framework.
> 
> For clarity, this patch contains only the common framework to
> support the remote unified launchers. The remote unified
> launchers will be added in separated patches.
> 
> The reasons why I duplicated some of the classes were:
> 
> - The remote unified launcher won't necessarily work just like
> the local unified. Some launch options (like choosing which
> remote framework to use, like RSE, Remotetools, git ...) can change
> the way the remote unified work in the future. However I haven't
> duplicated all classes - the remote framework shares a lot of
> code with the local one.
> 
> - Most of the APIs from the local unified framework are so tight
> with the classes and return types used in the local launchers that
> it would require a lot of code to make it work with the remote
> launchers and it would be error prone. I really didn't want to take
> the risk to add bugs in working code and, as I mentioned above,
> the remote framework can diverge from the local one when more
> remote options and support will be added in the future.
> 
> Change-Id: Id934a4572477dc2cb5193ff144d92af843c94173
> Reviewed-on: https://git.eclipse.org/r/9239
> Tested-by: Hudson CI
> Reviewed-by: Otavio Pontes <obusatto@xxxxxxxxxxxxxxxxxx>
> IP-Clean: Otavio Pontes <obusatto@xxxxxxxxxxxxxxxxxx>
> Tested-by: Otavio Pontes <obusatto@xxxxxxxxxxxxxxxxxx>
> ---
> M profiling/org.eclipse.linuxtools.profiling.launch/plugin.xml
> M
> profiling/org.eclipse.linuxtools.profiling.launch/schema/RemoteProxyManager.exsd
> A
> profiling/org.eclipse.linuxtools.profiling.launch/src/org/eclipse/linuxtools/internal/profiling/launch/provider/remote/AbstractProviderPreferencesPage.java
> A
> profiling/org.eclipse.linuxtools.profiling.launch/src/org/eclipse/linuxtools/internal/profiling/launch/provider/remote/AbstractProviderPropertyTab.java
> A
> profiling/org.eclipse.linuxtools.profiling.launch/src/org/eclipse/linuxtools/internal/profiling/launch/provider/remote/ProviderLaunchConfigurationTabGroup.java
> A
> profiling/org.eclipse.linuxtools.profiling.launch/src/org/eclipse/linuxtools/internal/profiling/launch/provider/remote/ProviderOptionsTab.java
> A
> profiling/org.eclipse.linuxtools.profiling.launch/src/org/eclipse/linuxtools/internal/profiling/launch/provider/remote/ProviderProfileConstants.java
> A
> profiling/org.eclipse.linuxtools.profiling.launch/src/org/eclipse/linuxtools/internal/profiling/launch/provider/remote/TimingPropertyTab.java
> A
> profiling/org.eclipse.linuxtools.profiling.launch/src/org/eclipse/linuxtools/internal/profiling/launch/provider/remote/launch/ProviderFramework.java
> A
> profiling/org.eclipse.linuxtools.profiling.launch/src/org/eclipse/linuxtools/internal/profiling/launch/provider/remote/launch/ProviderLaunchConfigurationDelegate.java
> M
> profiling/org.eclipse.linuxtools.profiling.launch/src/org/eclipse/linuxtools/profiling/launch/ProfileLaunchConfigurationTabGroup.java
> 11 files changed, 1,592 insertions(+), 3 deletions(-)
> 
> Approvals:
>   Otavio Pontes: Verified; Looks good to me, approved; IP review
>   completed
>   Hudson CI: Verified
> 
> 
> --
> To view, visit https://git.eclipse.org/r/9239
> To unsubscribe, visit https://git.eclipse.org/r/settings
> 
> Gerrit-MessageType: merged
> Gerrit-Change-Id: Id934a4572477dc2cb5193ff144d92af843c94173
> Gerrit-PatchSet: 2
> Gerrit-Project: linuxtools/org.eclipse.linuxtools
> Gerrit-Branch: master
> Gerrit-Owner: Daniel Henrique Barboza <danielhb@xxxxxxxxxx>
> Gerrit-Reviewer: Hudson CI
> Gerrit-Reviewer: Otavio Pontes <obusatto@xxxxxxxxxxxxxxxxxx>
>