Eclipse Community Forums
Forum Search:

Search      Help    Register    Login    Home
Home » Eclipse Projects » Buckminster dev » Subversive & Subclipse plugin logic duplicated
Subversive & Subclipse plugin logic duplicated [message #28909] Sun, 30 November 2008 10:11 Go to next message
Guillaume Chatelet is currently offline Guillaume Chatelet
Messages: 146
Registered: July 2009
Senior Member
Hello,

While trying to implement an internationalized handling of the thrown exceptions of subversion's plugin I remarked both Subclipse and Subversive plugins are quite duplicated code. This makes the two plugins much harder to maintain and their behavior might be inconsistent (which is the case actually). As a developer I dislike duplicated code much and I would like to get the common parts to be unique.

I would like to get the opinion of the Buckminster's developer community regarding this issue. I can create a parent plugin for subversive and subclipse to gather common parts.

Let me know what you think about this.

Best regards,
Guillaume
Re: Subversive & Subclipse plugin logic duplicated [message #28951 is a reply to message #28909] Sun, 30 November 2008 20:31 Go to previous messageGo to next message
Guillaume Chatelet is currently offline Guillaume Chatelet
Messages: 146
Registered: July 2009
Senior Member
If anyone is working on Subclipse & Subversive plugins please let me know so I could start refactoring without breaking your work.

Best regards,
Guillaume

Guillaume Chatelet wrote :
> Hello,
>
> While trying to implement an internationalized handling of the thrown exceptions of subversion's plugin I remarked both Subclipse and Subversive plugins are quite duplicated code. This makes the two plugins much harder to maintain and their behavior might be inconsistent (which is the case actually). As a developer I dislike duplicated code much and I would like to get the common parts to be unique.
>
> I would like to get the opinion of the Buckminster's developer community regarding this issue. I can create a parent plugin for subversive and subclipse to gather common parts.
>
> Let me know what you think about this.
>
> Best regards,
> Guillaume
Re: Subversive & Subclipse plugin logic duplicated [message #28989 is a reply to message #28909] Mon, 01 December 2008 07:59 Go to previous messageGo to next message
Thomas Hallgren is currently offline Thomas Hallgren
Messages: 3229
Registered: July 2009
Senior Member
Hi Guillaume,
The reason for the duplicated code is mostly historical. The Subversive
plug-in was provided to us as an external contribution and for them it
was the easiest way to do it.

I fully agree that the code would benefit largely from being refactored
so I'm all in favour if you want to do this.

Regards,
Thomas Hallgren


Guillaume Chatelet wrote:
> Hello,
>
> While trying to implement an internationalized handling of the thrown exceptions of subversion's plugin I remarked both Subclipse and Subversive plugins are quite duplicated code. This makes the two plugins much harder to maintain and their behavior might be inconsistent (which is the case actually). As a developer I dislike duplicated code much and I would like to get the common parts to be unique.
>
> I would like to get the opinion of the Buckminster's developer community regarding this issue. I can create a parent plugin for subversive and subclipse to gather common parts.
>
> Let me know what you think about this.
>
> Best regards,
> Guillaume
Re: Subversive & Subclipse plugin logic duplicated [message #29027 is a reply to message #28989] Thu, 04 December 2008 17:22 Go to previous messageGo to next message
Guillaume Chatelet is currently offline Guillaume Chatelet
Messages: 146
Registered: July 2009
Senior Member
Hi Thomas,

I spent a few hours of my (not so) spare time refactoring the plugins. This is a rather complicated task as the two plugins are dealing with different types with no common interface. I hope it to be finished in the next few days. I'll let you know.

Regards,
Guillaume

Thomas Hallgren wrote:
> Hi Guillaume,
> The reason for the duplicated code is mostly historical. The Subversive
> plug-in was provided to us as an external contribution and for them it
> was the easiest way to do it.
>
> I fully agree that the code would benefit largely from being refactored
> so I'm all in favour if you want to do this.
>
> Regards,
> Thomas Hallgren
>
>
> Guillaume Chatelet wrote:
>> Hello,
>>
>> While trying to implement an internationalized handling of the thrown
>> exceptions of subversion's plugin I remarked both Subclipse and
>> Subversive plugins are quite duplicated code. This makes the two
>> plugins much harder to maintain and their behavior might be
>> inconsistent (which is the case actually). As a developer I dislike
>> duplicated code much and I would like to get the common parts to be
>> unique.
>>
>> I would like to get the opinion of the Buckminster's developer
>> community regarding this issue. I can create a parent plugin for
>> subversive and subclipse to gather common parts.
>>
>> Let me know what you think about this.
>>
>> Best regards,
>> Guillaume
Re: Subversive & Subclipse plugin logic duplicated [message #29065 is a reply to message #29027] Tue, 09 December 2008 07:27 Go to previous messageGo to next message
Guillaume Chatelet is currently offline Guillaume Chatelet
Messages: 146
Registered: July 2009
Senior Member
Ok, this is it. I put a first version of the refactoring to the trunk.
I did my best not to introduce any bugs but I know by experience this is rather impossible.

I tested a few resolution + materialization including the buckminster-dev query, but I need more tests to be confident in the code.
I tested with Subclipse plugin but I need the other plugin to be tested also.

This is the first layer allowing us to handle correctly the i18n for SVN I'll try to add in the next days.

Best regards,
Guillaume

Guillaume CHATELET a écrit :
> Hi Thomas,
>
> I spent a few hours of my (not so) spare time refactoring the plugins.
> This is a rather complicated task as the two plugins are dealing with
> different types with no common interface. I hope it to be finished in
> the next few days. I'll let you know.
>
> Regards,
> Guillaume
>
> Thomas Hallgren wrote:
>> Hi Guillaume,
>> The reason for the duplicated code is mostly historical. The
>> Subversive plug-in was provided to us as an external contribution and
>> for them it was the easiest way to do it.
>>
>> I fully agree that the code would benefit largely from being
>> refactored so I'm all in favour if you want to do this.
>>
>> Regards,
>> Thomas Hallgren
>>
>>
>> Guillaume Chatelet wrote:
>>> Hello,
>>>
>>> While trying to implement an internationalized handling of the thrown
>>> exceptions of subversion's plugin I remarked both Subclipse and
>>> Subversive plugins are quite duplicated code. This makes the two
>>> plugins much harder to maintain and their behavior might be
>>> inconsistent (which is the case actually). As a developer I dislike
>>> duplicated code much and I would like to get the common parts to be
>>> unique.
>>>
>>> I would like to get the opinion of the Buckminster's developer
>>> community regarding this issue. I can create a parent plugin for
>>> subversive and subclipse to gather common parts.
>>>
>>> Let me know what you think about this.
>>>
>>> Best regards,
>>> Guillaume
Re: Subversive & Subclipse plugin logic duplicated [message #29103 is a reply to message #29065] Tue, 09 December 2008 07:52 Go to previous messageGo to next message
Thomas Hallgren is currently offline Thomas Hallgren
Messages: 3229
Registered: July 2009
Senior Member
Hi Guillaume,
Great! I have Subversive set up on my machine so I can test that.

Did you update the relevant features with the new plug-in? The CSPEC of
the external update site needs to be update too I think. I can do all
that if you are uncertain. Let me know in that case.

Regards,
Thomas Hallgren

Guillaume Chatelet wrote:
> Ok, this is it. I put a first version of the refactoring to the trunk.
> I did my best not to introduce any bugs but I know by experience this is rather impossible.
>
> I tested a few resolution + materialization including the buckminster-dev query, but I need more tests to be confident in the code.
> I tested with Subclipse plugin but I need the other plugin to be tested also.
>
> This is the first layer allowing us to handle correctly the i18n for SVN I'll try to add in the next days.
>
> Best regards,
> Guillaume
>
> Guillaume CHATELET a écrit :
>> Hi Thomas,
>>
>> I spent a few hours of my (not so) spare time refactoring the plugins.
>> This is a rather complicated task as the two plugins are dealing with
>> different types with no common interface. I hope it to be finished in
>> the next few days. I'll let you know.
>>
>> Regards,
>> Guillaume
>>
>> Thomas Hallgren wrote:
>>> Hi Guillaume,
>>> The reason for the duplicated code is mostly historical. The
>>> Subversive plug-in was provided to us as an external contribution and
>>> for them it was the easiest way to do it.
>>>
>>> I fully agree that the code would benefit largely from being
>>> refactored so I'm all in favour if you want to do this.
>>>
>>> Regards,
>>> Thomas Hallgren
>>>
>>>
>>> Guillaume Chatelet wrote:
>>>> Hello,
>>>>
>>>> While trying to implement an internationalized handling of the thrown
>>>> exceptions of subversion's plugin I remarked both Subclipse and
>>>> Subversive plugins are quite duplicated code. This makes the two
>>>> plugins much harder to maintain and their behavior might be
>>>> inconsistent (which is the case actually). As a developer I dislike
>>>> duplicated code much and I would like to get the common parts to be
>>>> unique.
>>>>
>>>> I would like to get the opinion of the Buckminster's developer
>>>> community regarding this issue. I can create a parent plugin for
>>>> subversive and subclipse to gather common parts.
>>>>
>>>> Let me know what you think about this.
>>>>
>>>> Best regards,
>>>> Guillaume
Re: Subversive & Subclipse plugin logic duplicated [message #29140 is a reply to message #29103] Tue, 09 December 2008 08:05 Go to previous messageGo to next message
Guillaume Chatelet is currently offline Guillaume Chatelet
Messages: 146
Registered: July 2009
Senior Member
No features nor CSPEC update sorry. I solely added my plugin to the list of dependencies of the subclipse & subversive plugins.
I would greatly appreciate if you could do that for me.

Please note the code need comments and even more refactoring can be done but at least the common parts are now gathered in a same file.
Still work to do so... I think my code would benefit from peer reviewing so if you feel like it, you're more than welcome. It would be good also to break big portions of code into more fine grained ones thus providing better readability.

Thomas Hallgren a écrit :
> Hi Guillaume,
> Great! I have Subversive set up on my machine so I can test that.
>
> Did you update the relevant features with the new plug-in? The CSPEC of
> the external update site needs to be update too I think. I can do all
> that if you are uncertain. Let me know in that case.
>
> Regards,
> Thomas Hallgren
>
> Guillaume Chatelet wrote:
>> Ok, this is it. I put a first version of the refactoring to the trunk.
>> I did my best not to introduce any bugs but I know by experience this
>> is rather impossible.
>>
>> I tested a few resolution + materialization including the
>> buckminster-dev query, but I need more tests to be confident in the code.
>> I tested with Subclipse plugin but I need the other plugin to be
>> tested also.
>>
>> This is the first layer allowing us to handle correctly the i18n for
>> SVN I'll try to add in the next days.
>>
>> Best regards,
>> Guillaume
>>
>> Guillaume CHATELET a écrit :
>>> Hi Thomas,
>>>
>>> I spent a few hours of my (not so) spare time refactoring the plugins.
>>> This is a rather complicated task as the two plugins are dealing with
>>> different types with no common interface. I hope it to be finished in
>>> the next few days. I'll let you know.
>>>
>>> Regards,
>>> Guillaume
>>>
>>> Thomas Hallgren wrote:
>>>> Hi Guillaume,
>>>> The reason for the duplicated code is mostly historical. The
>>>> Subversive plug-in was provided to us as an external contribution and
>>>> for them it was the easiest way to do it.
>>>>
>>>> I fully agree that the code would benefit largely from being
>>>> refactored so I'm all in favour if you want to do this.
>>>>
>>>> Regards,
>>>> Thomas Hallgren
>>>>
>>>>
>>>> Guillaume Chatelet wrote:
>>>>> Hello,
>>>>>
>>>>> While trying to implement an internationalized handling of the thrown
>>>>> exceptions of subversion's plugin I remarked both Subclipse and
>>>>> Subversive plugins are quite duplicated code. This makes the two
>>>>> plugins much harder to maintain and their behavior might be
>>>>> inconsistent (which is the case actually). As a developer I dislike
>>>>> duplicated code much and I would like to get the common parts to be
>>>>> unique.
>>>>>
>>>>> I would like to get the opinion of the Buckminster's developer
>>>>> community regarding this issue. I can create a parent plugin for
>>>>> subversive and subclipse to gather common parts.
>>>>>
>>>>> Let me know what you think about this.
>>>>>
>>>>> Best regards,
>>>>> Guillaume
>
Re: Subversive & Subclipse plugin logic duplicated [message #29178 is a reply to message #29140] Tue, 09 December 2008 11:22 Go to previous messageGo to next message
Thomas Hallgren is currently offline Thomas Hallgren
Messages: 3229
Registered: July 2009
Senior Member
OK, I updated the needed features. I also reverted some externalized
parameterized strings used in debug log output. Reason being that the
NLS will format the strings even when debug is turned off. Debug output
is not intended for the end user anyway so we simply exempt those
strings from externalization.

I found one NPE problem related to the shareProject method in the reader
type. The old implementation created the common roots using a static
method and a session instance was not obtained until after that had
happened. Now, the session is initialized before the roots are created
which is bad. It results in a null m_repositoryLocation in the
GenericSession constructor since there are no known repositories. That
in turn leads to the following:

java.lang.NullPointerException
at
org.eclipse.buckminster.subversive.internal.SubversiveReader Type.updateRepositoryMap(SubversiveReaderType.java:134)
at
org.eclipse.buckminster.subversion.GenericReaderType.sharePr oject(GenericReaderType.java:81)
at
org.eclipse.buckminster.core.materializer.WorkspaceMateriali zer.createProjectBinding(WorkspaceMaterializer.java:366)

I did a peer review on the bundles and other then what I just mentioned
you seem to have done a great job to refactor the bundles. Thanks a
bundle :)

Regards,
Thomas Hallgren



Guillaume Chatelet wrote:
> No features nor CSPEC update sorry. I solely added my plugin to the list of dependencies of the subclipse & subversive plugins.
> I would greatly appreciate if you could do that for me.
>
> Please note the code need comments and even more refactoring can be done but at least the common parts are now gathered in a same file.
> Still work to do so... I think my code would benefit from peer reviewing so if you feel like it, you're more than welcome. It would be good also to break big portions of code into more fine grained ones thus providing better readability.
>
> Thomas Hallgren a écrit :
>> Hi Guillaume,
>> Great! I have Subversive set up on my machine so I can test that.
>>
>> Did you update the relevant features with the new plug-in? The CSPEC of
>> the external update site needs to be update too I think. I can do all
>> that if you are uncertain. Let me know in that case.
>>
>> Regards,
>> Thomas Hallgren
>>
Re: Subversive & Subclipse plugin logic duplicated [message #29215 is a reply to message #29178] Tue, 09 December 2008 22:38 Go to previous messageGo to next message
Thomas Hallgren is currently offline Thomas Hallgren
Messages: 3229
Registered: July 2009
Senior Member
Hi Guillaume,
I did two more changes:

1. I discovered that the o.e.buckminster.subversion.commons and
o.e.buckminster.subclipse had dependencies to the subversive bundle
o.e.team.svn.core. That means that in order to run Subclipse, you need
to first install Subversive. I refactored the code so that this
dependency was removed.

2. The top package or o.e.buckminster.subversion.commons was named
o.e.buckminster.subversion (i.e. no commons at the end) so I renamed the
plug-in to match the top package name. This is a convention that we are
using throughout. Since the plug-in itself was renamed, so was the
folder that it resides in so take extra care next time you update your
workspace.

With those changes in place, all tests that I have done seems to go
through without any problems so I'll publish a new version now.

Regards,
Thomas Hallgren


Thomas Hallgren wrote:
> OK, I updated the needed features. I also reverted some externalized
> parameterized strings used in debug log output. Reason being that the
> NLS will format the strings even when debug is turned off. Debug output
> is not intended for the end user anyway so we simply exempt those
> strings from externalization.
>
> I found one NPE problem related to the shareProject method in the reader
> type. The old implementation created the common roots using a static
> method and a session instance was not obtained until after that had
> happened. Now, the session is initialized before the roots are created
> which is bad. It results in a null m_repositoryLocation in the
> GenericSession constructor since there are no known repositories. That
> in turn leads to the following:
>
> java.lang.NullPointerException
> at
> org.eclipse.buckminster.subversive.internal.SubversiveReader Type.updateRepositoryMap(SubversiveReaderType.java:134)
>
> at
> org.eclipse.buckminster.subversion.GenericReaderType.sharePr oject(GenericReaderType.java:81)
>
> at
> org.eclipse.buckminster.core.materializer.WorkspaceMateriali zer.createProjectBinding(WorkspaceMaterializer.java:366)
>
>
> I did a peer review on the bundles and other then what I just mentioned
> you seem to have done a great job to refactor the bundles. Thanks a
> bundle :)
>
> Regards,
> Thomas Hallgren
>
>
>
> Guillaume Chatelet wrote:
>> No features nor CSPEC update sorry. I solely added my plugin to the
>> list of dependencies of the subclipse & subversive plugins.
>> I would greatly appreciate if you could do that for me.
>>
>> Please note the code need comments and even more refactoring can be
>> done but at least the common parts are now gathered in a same file.
>> Still work to do so... I think my code would benefit from peer
>> reviewing so if you feel like it, you're more than welcome. It would
>> be good also to break big portions of code into more fine grained ones
>> thus providing better readability.
>>
>> Thomas Hallgren a écrit :
>>> Hi Guillaume,
>>> Great! I have Subversive set up on my machine so I can test that.
>>>
>>> Did you update the relevant features with the new plug-in? The CSPEC of
>>> the external update site needs to be update too I think. I can do all
>>> that if you are uncertain. Let me know in that case.
>>>
>>> Regards,
>>> Thomas Hallgren
>>>
Re: Subversive & Subclipse plugin logic duplicated [message #29253 is a reply to message #29215] Tue, 09 December 2008 22:51 Go to previous message
Guillaume Chatelet is currently offline Guillaume Chatelet
Messages: 146
Registered: July 2009
Senior Member
Great Thomas !

You're right about the two points you mentioned.

Regarding 2: actually I named my commons plugin o.e.buckminster.subversion at first and then I renamed it to make it more explicit : this explains the mistakes.

Thank you very much for reviewing my code. I planned to do some more tests today but I haven't been able to... Days are too short !
I hope you haven't discovered awful things in my code ^_^

Best regards,
Guillaume

Thomas Hallgren a écrit :
> Hi Guillaume,
> I did two more changes:
>
> 1. I discovered that the o.e.buckminster.subversion.commons and
> o.e.buckminster.subclipse had dependencies to the subversive bundle
> o.e.team.svn.core. That means that in order to run Subclipse, you need
> to first install Subversive. I refactored the code so that this
> dependency was removed.
>
> 2. The top package or o.e.buckminster.subversion.commons was named
> o.e.buckminster.subversion (i.e. no commons at the end) so I renamed the
> plug-in to match the top package name. This is a convention that we are
> using throughout. Since the plug-in itself was renamed, so was the
> folder that it resides in so take extra care next time you update your
> workspace.
>
> With those changes in place, all tests that I have done seems to go
> through without any problems so I'll publish a new version now.
>
> Regards,
> Thomas Hallgren
>
>
> Thomas Hallgren wrote:
>> OK, I updated the needed features. I also reverted some externalized
>> parameterized strings used in debug log output. Reason being that the
>> NLS will format the strings even when debug is turned off. Debug
>> output is not intended for the end user anyway so we simply exempt
>> those strings from externalization.
>>
>> I found one NPE problem related to the shareProject method in the
>> reader type. The old implementation created the common roots using a
>> static method and a session instance was not obtained until after that
>> had happened. Now, the session is initialized before the roots are
>> created which is bad. It results in a null m_repositoryLocation in the
>> GenericSession constructor since there are no known repositories. That
>> in turn leads to the following:
>>
>> java.lang.NullPointerException
>> at
>> org.eclipse.buckminster.subversive.internal.SubversiveReader Type.updateRepositoryMap(SubversiveReaderType.java:134)
>>
>> at
>> org.eclipse.buckminster.subversion.GenericReaderType.sharePr oject(GenericReaderType.java:81)
>>
>> at
>> org.eclipse.buckminster.core.materializer.WorkspaceMateriali zer.createProjectBinding(WorkspaceMaterializer.java:366)
>>
>>
>> I did a peer review on the bundles and other then what I just
>> mentioned you seem to have done a great job to refactor the bundles.
>> Thanks a bundle :)
>>
>> Regards,
>> Thomas Hallgren
>>
>>
>>
>> Guillaume Chatelet wrote:
>>> No features nor CSPEC update sorry. I solely added my plugin to the
>>> list of dependencies of the subclipse & subversive plugins.
>>> I would greatly appreciate if you could do that for me.
>>>
>>> Please note the code need comments and even more refactoring can be
>>> done but at least the common parts are now gathered in a same file.
>>> Still work to do so... I think my code would benefit from peer
>>> reviewing so if you feel like it, you're more than welcome. It would
>>> be good also to break big portions of code into more fine grained
>>> ones thus providing better readability.
>>>
>>> Thomas Hallgren a écrit :
>>>> Hi Guillaume,
>>>> Great! I have Subversive set up on my machine so I can test that.
>>>>
>>>> Did you update the relevant features with the new plug-in? The CSPEC of
>>>> the external update site needs to be update too I think. I can do all
>>>> that if you are uncertain. Let me know in that case.
>>>>
>>>> Regards,
>>>> Thomas Hallgren
>>>>
>
Previous Topic:Is a tighter leash needed on projects participating in the next release train?
Next Topic:No more debug informations in last version ?
Goto Forum:
  


Current Time: Thu Oct 30 12:15:57 GMT 2014

Powered by FUDForum. Page generated in 0.02406 seconds
.:: Contact :: Home ::.

Powered by: FUDforum 3.0.2.
Copyright ©2001-2010 FUDforum Bulletin Board Software