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

[Otávio Pontes <obusatto@xxxxxxxxxxxxxxxxxx>]

commit reverted.

On 12/14/2012 11:24 AM, Aleksandar Kurtakov wrote:
> 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>