Home » Eclipse Projects » Buckminster dev » Subversive & Subclipse plugin logic duplicated
| |
| Re: Subversive & Subclipse plugin logic duplicated [message #28989 is a reply to message #28909] |
Mon, 01 December 2008 07:59   |
|
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 |
 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 |
|
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 |
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 |
|
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 |
|
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 |
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
>>>>
>
|
|
|
Goto Forum:
Current Time: Tue Apr 23 12:07:35 GMT 2024
Powered by FUDForum. Page generated in 0.04318 seconds |
] 
Search
Help
Register
Login
Home
