Erasure of FIXME's and TODO's in today's commits

[CD...@hannaford.com]

Torsten,

I very much appreciated the work you and your group has done to create
Pluto 2.0, but I was very disappointed in many of the commits you did
today. Rather than fixing most of the TODOs and FIXMEs, you just erased
them. Some of the TODOs and FIXMEs are inconsequential and not necessary
and should be erased and some FIXMEs are not that important and should be
changed to TODOs. But many of these should have been fixed before the FIXME
or TODO was removed.

Here are a few specific comments:
1. Many of the TODOs are those automatically added by Eclipse and are
labelled "Auto-generated catch block" and include a printStackTrace() call
to stdout. This should have been changed to log to Pluto's log file
including a relevant message and the exception so the stack trace will
appear in the Pluto log. Consideration should also be made to rethrow the
exception so it is propagated up the stack trace to the user.
2. The FIXME in SupportedModesServiceImpl.isPortletManagedMode() refers to
the fact that this method is not properly implemented and had notes on how
to implement the method. You just erased the FIXME and notes rather than
fixing the method as detailed in the FIXME comment.
3. In PortletURLTag286.doStartTag(), rather than implementing the FIXME
suggestion, you just erased the FIXME and the suggested code that was
commented out.

I think the commits where FIXMEs and TODOs are just erased should be rolled
back except in those cases where the FIXME or TODO does not add value or
the issue pointed out by the FIXME or TODO was fixed. Does anybody else
have this opinion or am I making too much out of nothing here?

Re: Erasure of FIXME's and TODO's in today's commits

[Christian Raschka <ch...@informatik.uni-jena.de>]

Ate, I'm feeling with you ;-) Please remember me to the guys of the
JSR-301 EG. Sadly it wasn't possible for me to take part in the F2F.

I've had a look at the patch and there is much work done by Torsten,
implementing missing things and cleaning up. Thank you Torsten.

Nevertheless, there are FIXMEs and TODOs that shouldn't be removed and
these should be rolled back. Some of them are from 1.1.x and should be
reviewed if they could be removed safely.

If there are JIRA issues for the big problems without solutions, I can
help with some of these.

Cheers,
   Christian

Re: Erasure of FIXME's and TODO's in today's commits

[at...@douma.nu]

I'm currently not able to check on these changes as I'm at a JSR-301 F2F
and can't get my laptop to connect to the internet (I'm typing this
response at a public booth) which it looks like will remain so the next 2
days :(

Anyway, if this is all true, I fully agree these changes need to be rolled
back. Its clear there are areas in Pluto 2.0 trunk which needs fixing and
implementing before it can be considered usable and more or less complete.
Simply ripping out FIXME and TODO comment won't bring us any closer to
that, on the contrary...

My suggestion is th following:
- rollback these changes
- make a list of all *must* have issues/FIXMEs
- make a list of all *nice* to have TODOs
- if need be, discuss these on the dev list first
- create JIRA issues for all the above, making (at least) the first list
items required for the 2.0 container release

Regards,

Ate

>
> Torsten,
>
> I very much appreciated the work you and your group has done to create
> Pluto 2.0, but I was very disappointed in many of the commits you did
> today. Rather than fixing most of the TODOs and FIXMEs, you just erased
> them. Some of the TODOs and FIXMEs are inconsequential and not necessary
> and should be erased and some FIXMEs are not that important and should be
> changed to TODOs. But many of these should have been fixed before the
> FIXME
> or TODO was removed.
>
> Here are a few specific comments:
> 1. Many of the TODOs are those automatically added by Eclipse and are
> labelled "Auto-generated catch block" and include a printStackTrace() call
> to stdout. This should have been changed to log to Pluto's log file
> including a relevant message and the exception so the stack trace will
> appear in the Pluto log. Consideration should also be made to rethrow the
> exception so it is propagated up the stack trace to the user.
> 2. The FIXME in SupportedModesServiceImpl.isPortletManagedMode() refers to
> the fact that this method is not properly implemented and had notes on how
> to implement the method. You just erased the FIXME and notes rather than
> fixing the method as detailed in the FIXME comment.
> 3. In PortletURLTag286.doStartTag(), rather than implementing the FIXME
> suggestion, you just erased the FIXME and the suggested code that was
> commented out.
>
> I think the commits where FIXMEs and TODOs are just erased should be
> rolled
> back except in those cases where the FIXME or TODO does not add value or
> the issue pointed out by the FIXME or TODO was fixed. Does anybody else
> have this opinion or am I making too much out of nothing here?
>

Re: Erasure of FIXME's and TODO's in today's commits

[Carsten Ziegeler <cz...@apache.org>]

CDoremus@hannaford.com wrote:
> Torsten,
> 
> I very much appreciated the work you and your group has done to create
> Pluto 2.0, but I was very disappointed in many of the commits you did
> today. Rather than fixing most of the TODOs and FIXMEs, you just erased
> them. Some of the TODOs and FIXMEs are inconsequential and not necessary
> and should be erased and some FIXMEs are not that important and should be
> changed to TODOs. But many of these should have been fixed before the FIXME
> or TODO was removed.
> 
> Here are a few specific comments:
> 1. Many of the TODOs are those automatically added by Eclipse and are
> labelled "Auto-generated catch block" and include a printStackTrace() call
> to stdout. This should have been changed to log to Pluto's log file
> including a relevant message and the exception so the stack trace will
> appear in the Pluto log. Consideration should also be made to rethrow the
> exception so it is propagated up the stack trace to the user.
> 2. The FIXME in SupportedModesServiceImpl.isPortletManagedMode() refers to
> the fact that this method is not properly implemented and had notes on how
> to implement the method. You just erased the FIXME and notes rather than
> fixing the method as detailed in the FIXME comment.
> 3. In PortletURLTag286.doStartTag(), rather than implementing the FIXME
> suggestion, you just erased the FIXME and the suggested code that was
> commented out.
> 
> I think the commits where FIXMEs and TODOs are just erased should be rolled
> back except in those cases where the FIXME or TODO does not add value or
> the issue pointed out by the FIXME or TODO was fixed. Does anybody else
> have this opinion or am I making too much out of nothing here?
> 
I totally agree.

Carsten

-- 
Carsten Ziegeler
cziegeler@apache.org

Re: Erasure of FIXME's and TODO's in today's commits

[csev <cs...@umich.edu>]

On Jan 15, 2008, at 9:33 AM, CDoremus@hannaford.com wrote:

> I think the commits where FIXMEs and TODOs are just erased should be  
> rolled
> back except in those cases where the FIXME or TODO does not add  
> value or
> the issue pointed out by the FIXME or TODO was fixed. Does anybody  
> else
> have this opinion or am I making too much out of nothing here?

I am not a committer - but my general opinion on these things is to  
leave them in 100% unless someone has carefully analyzed each one and  
produced a solution or made sure that they could be deleted.

There is no harm in having some TODOs and FIXMEs sitting there in  
trunk - even for quite some time.  Their very existence or the  
information that they have will help some programmer in the future who  
stumbles across a big or issue.

I say leave them in and take them out slowly thinking about each one.   
If there is a whole class - (like the printstack ones) go ahead and  
fix en masse.

But don't just remove en-masse IMO.

Chuck Severance