You are viewing a plain text version of this content. The canonical link for it is here.
Posted to fop-dev@xmlgraphics.apache.org by Jeremias Maerki <de...@jeremias-maerki.ch> on 2010/08/12 14:25:33 UTC
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
I've now applied the patch locally and done a detailed review. I'm
posting this a bit outside the context of recent discussions to simply
state my present opinion after looking into the patch.
Generally, this is a big improvement. So thanks, Glenn, for your work
here!
I'm also not particularly happy about the //CS* comments. To a certain
degree I think I could live with them. A count shows 279 usages. I think
that may be a tad too much. Maybe we can find something in between, like
making more use of the "error" severity. Most checks are just warnings
right now. So using errors will make it easier to enforce at least the
rules most important to us. I've also experimented with the regular
expressions:
<module name="ConstantNameCheck">
<property name="format" value="(^[A-Z](_?[A-Z0-9]+)*$)|log"/>
<property name="severity" value="warning"/>
</module>
This should already make several such //CS comments unnecessary. There
are other comments referencing ConstantNameCheck where we should rather
convert the name to upper case. That will cut down on these even further.
Like Chris suggested, we could then even decide to live with a few
warnings as long as we increase the severity of the most important rules
and set up a no-go policy for "errors".
I saw some changes in LineBreakPairTable.txt and LineBreakUtils.java.
Glenn, was that an accidental overflow from your work on the new
features?
I have no problem with the sometimes rather generic Javadoc comments.
Every committer is invited to improve on those as he's passing over
particular code parts. I know that we have quite a bit of outdated
documentation already in our Javadocs. So these comments don't make the
situation worse IMO. The only thing we can do is gradually improve. But
at least the generic javadocs lets us cut down on the number of warnings
so we can really focus to improve there rather than capitulate before
thousands of warnings.
Finally a nit: some files have got method signatures with whitespace
before and after the parantheses. We don't traditionally do that but the
Checkstyle profile doesn't seem to catch that. I guess it would be safe
to add that rule so we can fix those occurences.
I would suggest the following as our next steps:
1. Clarify the thing with LineBreak*.
2. Decide (quickly, please) whether to remove the //CS comments or to
allow them for now and optionally do something about them later. (I'm
tending towards removing them but I don't have a problem if we do it the
other way.)
3. Commit the patch to Trunk more or less as is (pending //CS decision).
3. Adjust the Checkstyle profile to allow "log" and disallow whitespace
before and after parantheses. Then remove "log"-related //CS constants
and excessive whitespace.
4. Merge the changes into the Temp_ComplexScripts parts.
5. Glenn could then provide a new patch against the branch which we
could do a cursory review on. We apply that and experiment with what
he's built. He can continue his work.
6. We continue to incrementally improve our coding standards.
I'm happy to do the grunt work. Like Glenn, I don't like to hold
principle discussions right now because that holds up several people
from doing day-to-day work. That doesn't mean we can't hold them, but I
don't see why we have to do it as a precondition to processing this
patch. The patch gets us further but doesn't preclude any futher
improvements later.
Please, let's get this done.
Jeremias Maerki
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
Thanks, Vincent.
On 12.08.2010 18:31:50 Vincent Hennebert wrote:
> Hi,
>
> Jeremias Maerki wrote:
> > I've now applied the patch locally and done a detailed review. I'm
> > posting this a bit outside the context of recent discussions to simply
> > state my present opinion after looking into the patch.
> >
> > Generally, this is a big improvement. So thanks, Glenn, for your work
> > here!
> >
> > I'm also not particularly happy about the //CS* comments. To a certain
> > degree I think I could live with them. A count shows 279 usages. I think
> > that may be a tad too much. Maybe we can find something in between, like
> > making more use of the "error" severity. Most checks are just warnings
> > right now. So using errors will make it easier to enforce at least the
> > rules most important to us. I've also experimented with the regular
> > expressions:
> >
> > <module name="ConstantNameCheck">
> > <property name="format" value="(^[A-Z](_?[A-Z0-9]+)*$)|log"/>
> > <property name="severity" value="warning"/>
> > </module>
> >
> > This should already make several such //CS comments unnecessary. There
> > are other comments referencing ConstantNameCheck where we should rather
> > convert the name to upper case. That will cut down on these even further.
> > Like Chris suggested, we could then even decide to live with a few
> > warnings as long as we increase the severity of the most important rules
> > and set up a no-go policy for "errors".
>
> (This is precisely why I suggested that we agree on an improved
> Checkstyle first, to avoid introducing unnecessary //CS comments.)
You sounded like you wanted to discuss this forwards and backwards
before going through with the patch. We only need to agree on some key
aspects and can deal with the finer points later.
> I don’t really have an opinion about that. Since zero-warning won’t be
> achieved in the short term anyway, I suppose we could remove them for
> now. Once we decide to enforce a zero-warning policy then they will
> probably have to be used, along with a TODO warning indicated that this
> is old code that needs refactoring; and thus make the difference with
> new CSOK comments introduced later on with due care.
So, if I get this right, we have one person (Glenn) for the //CS
comments, and 3 people (Vincent, Chris and me) more or less against them.
Glenn, I hope you can live with it if I remove them when processing the
patch tomorrow. I'll try to get rid of some of the issues in a second
step so we get the warnings down to a "comfortable" level.
> > I saw some changes in LineBreakPairTable.txt and LineBreakUtils.java.
> > Glenn, was that an accidental overflow from your work on the new
> > features?
> >
> > I have no problem with the sometimes rather generic Javadoc comments.
> > Every committer is invited to improve on those as he's passing over
> > particular code parts. I know that we have quite a bit of outdated
> > documentation already in our Javadocs. So these comments don't make the
> > situation worse IMO. The only thing we can do is gradually improve. But
> > at least the generic javadocs lets us cut down on the number of warnings
> > so we can really focus to improve there rather than capitulate before
> > thousands of warnings.
>
> I’m ok with that. Some less generic comments will need to be
> double-checked. Not that I don’t trust Glenn on that matter, but some
> parts of the code (especially layout) are tricky and it’s very easy to
> be mistaken. And I think a wrong Javadoc comment does more harm than no
> comment at all.
...if they are ever read at all. ;-) But you're right essentially. As I
said: this will always be work/improvements in progress.
> > Finally a nit: some files have got method signatures with whitespace
> > before and after the parantheses. We don't traditionally do that but the
> > Checkstyle profile doesn't seem to catch that. I guess it would be safe
> > to add that rule so we can fix those occurences.
>
> +1
>
>
> > I would suggest the following as our next steps:
> >
> > 1. Clarify the thing with LineBreak*.
> > 2. Decide (quickly, please) whether to remove the //CS comments or to
> > allow them for now and optionally do something about them later. (I'm
> > tending towards removing them but I don't have a problem if we do it the
> > other way.)
>
> +1 for removing them for now.
>
>
> > 3. Commit the patch to Trunk more or less as is (pending //CS decision).
>
> -1, among other things there are deprecated flags/methods that were
> removed and I feel that that must be discussed first (mainly
> Graphics2DAdapter.paintImage).
Sorry, missed that. I've still not changed Barcode4J so the current
B4J release would probably fail if this method is removed. I guess I
should finally fix that.
Renderer.startPageSequence(LineArea) can probably safely be removed
(Deprecation since Jan 2008). But we can leave that to decide for later.
> > 3. Adjust the Checkstyle profile to allow "log" and disallow whitespace
> > before and after parantheses. Then remove "log"-related //CS constants
> > and excessive whitespace.
>
> +0, I would just put log in uppercase but I don’t really mind.
Probably better to put it in uppercase. I'll do that.
>
> > 4. Merge the changes into the Temp_ComplexScripts parts.
> > 5. Glenn could then provide a new patch against the branch which we
> > could do a cursory review on. We apply that and experiment with what
> > he's built. He can continue his work.
> > 6. We continue to incrementally improve our coding standards.
> >
> > I'm happy to do the grunt work. Like Glenn, I don't like to hold
> > principle discussions right now because that holds up several people
> > from doing day-to-day work.
> > That doesn't mean we can't hold them, but I
> > don't see why we have to do it as a precondition to processing this
> > patch. The patch gets us further but doesn't preclude any futher
> > improvements later.
> >
> > Please, let's get this done.
>
> I’m not happy with that approach. When this topic was first mentioned
> [1] I did say that the Checkstyle file needed improvement and that until
> then this would be premature to work on that. My advice was not followed
> and now we should apply this patch ASAP without discussion? I’m not sure
> that trying to force things is a good way to get involved. The
> consensus-based approach inherent to any Apache project is not being
> followed here.
We have a de-facto consensus on the status quo with the wish to improve
on it at some point. I don't want to delay this patch any longer than
necessary as we have a quite competent new contributor who is not afraid
to dive into the layout engine. You were right to point out some details
which I'll address. But we can sort of further details later. I don't
see those plans as a showstopper for processing the patch (with
mentioned modifications).
> [1] http://markmail.org/message/bmbocjhmav3dlahc
>
> So now this patch is there, and it contains a lot of good stuff, and
> that’s great. But like I explained in my earlier message many things
> will have to be re-visited once a new Checkstyle file is put in place.
> So in the end this was a waste of time to do that now.
I don't see it like that. There are some issues with the patch which we
can address. Glenn's work still won't be in vain and he can continue
with his main task. Nothing's lost to address further issues later. Just
because we have some changes/improvements, we don't suddenly have to fix
everything in one run blocking the whole development process.
That Glenn has a budget from BasisTech to work on FOP is a really great
thing IMO. Basis is a silver sponsor for the ASF which says something
about their dedication. We need to make use of that resource. We can
really use all the resources we can get and we should try to make an
effort not to stand in the way of that. Of course that should not go as
far as to ignore our values or our concerns. I'm sure Glenn understands
that. And it's surely a good thing to balance out a bit where
contributions to FOP come from.
> I’ll list in a separate message the modifications that I feel need to be
> discussed before being applied (although I’ll be offline from tomorrow
> to Tuesday). For the rest, Jeremias, if you’re happy to apply them, feel
> free to proceed.
Thank you. I'll make an effort to address the concerns raised. And I'm
sure we're all ok to discuss further improvements to our code policies.
Jeremias Maerki
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc
warnings (a proposal for next steps)
Posted by Glenn Adams <gl...@skynav.com>.
On Fri, Aug 13, 2010 at 10:57 AM, Glenn Adams <gl...@skynav.com> wrote:
> 3. Adjust the Checkstyle profile to allow "log" and disallow whitespace
>> before and after parantheses. Then remove "log"-related //CS constants
>> and excessive whitespace.
>>
>
> I would not agree to restricting the style rules to prohibit whitespace
> before/after parentheses. I prefer *always* using whitespace around parens
> in Java (and C/C++).
>
Allow me to expand on this. I can accept such a rule (for prohibiting
whitespace before/after parens), but only if it is accepted that CSOFF be
used to disable that rule globally in the files of which I am the original
author. That is, I could agree to enforce and use that rule on files I did
not author, as long as I can avoid using that rule on the files I author.
By the way, this is precisely why there are going to always be limits to
obtaining consensus on style rules, particularly on those that are the most
stylistic in nature, of which I would suggest that whitespace distribution
will remain the most subjective.
What to do in such a case? Either don't impose the rule at all, or impose it
as a default while allowing overrides for those that do not concur.
There may indeed be some core set of rules where there is a true consensus
on application and enforcing, some of which may be related to whitespace.
For example, should tabs be permitted? Even though my editor can handle that
with appropriate embedded comments in the code, it is undesirable, and not
everyone uses the same editor. So best stay with NO TABS. On the other hand,
there are other possible rules for which a unanimous consensus will be
impossible, and for those, we can only not employ the rule or employ it
*merely* as a default, allowing overrides.
You will also now notice that we have descended onto that slippery slope of
discussing subjective preferences about style rules. I hope we can climb off
that slope soon and finish this patch in order to progress with useful
features.
G.
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc
warnings (a proposal for next steps)
Posted by Glenn Adams <gl...@skynav.com>.
inline
On Thu, Aug 12, 2010 at 8:25 PM, Jeremias Maerki <de...@jeremias-maerki.ch>wrote:
>
> 1. Clarify the thing with LineBreak*.
>
It was necessary to update the line break data in order to regenerate
LineBreakUtils.java; otherwise, the generation process failed to to a
missing column ('CP') when it attempts to pull down and reprocess the
Unicode property data.
> 2. Decide (quickly, please) whether to remove the //CS comments or to
> allow them for now and optionally do something about them later. (I'm
> tending towards removing them but I don't have a problem if we do it the
> other way.)
>
Removing them wastes the effort I made to track them down and make a
judgment call over each one. If you want to remove them, then I want a line
by line review of every one being removed, with justification for its
removal.
Removing them allows at least 279 warnings to remain, which is 279 too many.
The presence of more than zero warnings tells other committers and
developers that increasing the number of warnings is tolerated and accepted,
which is not a good message to send.
Removing them yields to the wrong idea that they should not be use, and that
there should be no exceptions made to the style rules. That is wrong
thinking in my opinion.
Having said the above, I would agree with removing them if the checkstyle
rules are changed so that their removal does not cause any warnings. That
would mean removing the following checks or adjusting them so that they were
not triggered:
ConstantNameCheck
FileLengthCheck
InnerAssiignmentCheck
LineLengthCheck
ParameterNumberCheck
MethodLengthCheck
VisibilityModifierCheck
I sent the full list out yesterday, but I have not seen any comments on
specifics. If this patch is going to be delayed to resolve this NOW instead
of gradually over time (the wiser choice) then, we need to review them all
and sign off on them.
I cannot accept a result that produces any warning output.
> 3. Commit the patch to Trunk more or less as is (pending //CS decision).
> 3. Adjust the Checkstyle profile to allow "log" and disallow whitespace
> before and after parantheses. Then remove "log"-related //CS constants
> and excessive whitespace.
>
I would not agree to restricting the style rules to prohibit whitespace
before/after parentheses. I prefer *always* using whitespace around parens
in Java (and C/C++).
> 4. Merge the changes into the Temp_ComplexScripts parts.
>
Yes, whatever the outcome of the warning cleanup, it should be merged into
that branch before applying the complex scripts patch. I will update that
patch once the cleanup merge occurs so that there are no merge problems,
then the updated patch can be used to populate that branch.
> 5. Glenn could then provide a new patch against the branch which we
> could do a cursory review on. We apply that and experiment with what
> he's built. He can continue his work.
>
Yes.
> 6. We continue to incrementally improve our coding standards.
Unfortunately, IMO, the increment being proposed here is larger than
necessary. The patch could be applied while leaving the CS* comments in
place without doing any damage to quality, and, in fact, improves quality by
recording the result of a careful audit. The group *could* if it chooses,
subsequently make incremental improvements to fine-tune the style rules and
removing some or even most of the CS* comments, but I reject the notion that
no CS suppressions should ever be used. That is an attempt to impose an
ideal to coding style for which we will not be able to obtain an absolute
consensus. The options are only as follows:
1. ignore warnings - which has been the status quo, and what we are
trying to fix here
2. choose such a loose set of style rules such that no warnings occur,
which may mean reducing the effectiveness of the tool and process;
3. take a pragmatic stance, using tighter rules but allow developers to
use common sense and intelligence in using CS* comments;
Of these options, #1 says do nothing, thus resulting in a waste of my time
and this exercise, and goes against the apparent wishes of most who have
commented positively on fixing this. Option #2 may allow achieving the goal
of zero warnings, but may prevent catching cases where some coding change is
warranted, e.g., catching field visibility issues. Option #3 permits the use
of tighter rules to catch potential problems, while giving developers the
tools they need to make informed choices about when to go outside of the
style guidelines.
My vote is #3.
G.
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc
warnings (a proposal for next steps)
Posted by Vincent Hennebert <vh...@gmail.com>.
Hi,
Jeremias Maerki wrote:
> I've now applied the patch locally and done a detailed review. I'm
> posting this a bit outside the context of recent discussions to simply
> state my present opinion after looking into the patch.
>
> Generally, this is a big improvement. So thanks, Glenn, for your work
> here!
>
> I'm also not particularly happy about the //CS* comments. To a certain
> degree I think I could live with them. A count shows 279 usages. I think
> that may be a tad too much. Maybe we can find something in between, like
> making more use of the "error" severity. Most checks are just warnings
> right now. So using errors will make it easier to enforce at least the
> rules most important to us. I've also experimented with the regular
> expressions:
>
> <module name="ConstantNameCheck">
> <property name="format" value="(^[A-Z](_?[A-Z0-9]+)*$)|log"/>
> <property name="severity" value="warning"/>
> </module>
>
> This should already make several such //CS comments unnecessary. There
> are other comments referencing ConstantNameCheck where we should rather
> convert the name to upper case. That will cut down on these even further.
> Like Chris suggested, we could then even decide to live with a few
> warnings as long as we increase the severity of the most important rules
> and set up a no-go policy for "errors".
(This is precisely why I suggested that we agree on an improved
Checkstyle first, to avoid introducing unnecessary //CS comments.)
I don’t really have an opinion about that. Since zero-warning won’t be
achieved in the short term anyway, I suppose we could remove them for
now. Once we decide to enforce a zero-warning policy then they will
probably have to be used, along with a TODO warning indicated that this
is old code that needs refactoring; and thus make the difference with
new CSOK comments introduced later on with due care.
> I saw some changes in LineBreakPairTable.txt and LineBreakUtils.java.
> Glenn, was that an accidental overflow from your work on the new
> features?
>
> I have no problem with the sometimes rather generic Javadoc comments.
> Every committer is invited to improve on those as he's passing over
> particular code parts. I know that we have quite a bit of outdated
> documentation already in our Javadocs. So these comments don't make the
> situation worse IMO. The only thing we can do is gradually improve. But
> at least the generic javadocs lets us cut down on the number of warnings
> so we can really focus to improve there rather than capitulate before
> thousands of warnings.
I’m ok with that. Some less generic comments will need to be
double-checked. Not that I don’t trust Glenn on that matter, but some
parts of the code (especially layout) are tricky and it’s very easy to
be mistaken. And I think a wrong Javadoc comment does more harm than no
comment at all.
> Finally a nit: some files have got method signatures with whitespace
> before and after the parantheses. We don't traditionally do that but the
> Checkstyle profile doesn't seem to catch that. I guess it would be safe
> to add that rule so we can fix those occurences.
+1
> I would suggest the following as our next steps:
>
> 1. Clarify the thing with LineBreak*.
> 2. Decide (quickly, please) whether to remove the //CS comments or to
> allow them for now and optionally do something about them later. (I'm
> tending towards removing them but I don't have a problem if we do it the
> other way.)
+1 for removing them for now.
> 3. Commit the patch to Trunk more or less as is (pending //CS decision).
-1, among other things there are deprecated flags/methods that were
removed and I feel that that must be discussed first (mainly
Graphics2DAdapter.paintImage).
> 3. Adjust the Checkstyle profile to allow "log" and disallow whitespace
> before and after parantheses. Then remove "log"-related //CS constants
> and excessive whitespace.
+0, I would just put log in uppercase but I don’t really mind.
> 4. Merge the changes into the Temp_ComplexScripts parts.
> 5. Glenn could then provide a new patch against the branch which we
> could do a cursory review on. We apply that and experiment with what
> he's built. He can continue his work.
> 6. We continue to incrementally improve our coding standards.
>
> I'm happy to do the grunt work. Like Glenn, I don't like to hold
> principle discussions right now because that holds up several people
> from doing day-to-day work.
> That doesn't mean we can't hold them, but I
> don't see why we have to do it as a precondition to processing this
> patch. The patch gets us further but doesn't preclude any futher
> improvements later.
>
> Please, let's get this done.
I’m not happy with that approach. When this topic was first mentioned
[1] I did say that the Checkstyle file needed improvement and that until
then this would be premature to work on that. My advice was not followed
and now we should apply this patch ASAP without discussion? I’m not sure
that trying to force things is a good way to get involved. The
consensus-based approach inherent to any Apache project is not being
followed here.
[1] http://markmail.org/message/bmbocjhmav3dlahc
So now this patch is there, and it contains a lot of good stuff, and
that’s great. But like I explained in my earlier message many things
will have to be re-visited once a new Checkstyle file is put in place.
So in the end this was a waste of time to do that now.
I’ll list in a separate message the modifications that I feel need to be
discussed before being applied (although I’ll be offline from tomorrow
to Tuesday). For the rest, Jeremias, if you’re happy to apply them, feel
free to proceed.
Vincent
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
Thanks for providing your view. So we have a similar view.
This gives me another shove in the butt to finally make the B4J 2.1
release. JEuclid should already be fine. I've seen that Max has already
switched to the other method. So, we can easily remove the deprecate
method before the next release but not really before I've fixed B4J as
people who work with FOP Trunk would run into problems.
On 13.08.2010 17:22:26 Simon Pepping wrote:
> I want to move forward with Glenn's work. As you wrote, it is an
> important addition to FOP which we cannot let go unused.
>
> I feel that the CHECKSTYLE comments are clear, and allow us to take
> any action later that we require. They could be harmful if we would
> feel that further work would have to be done, but actually is not
> done. Then the comments will hide that situation. But that is up to
> us.
>
> I understand that Glenn wants to work from a clean checkstyle and
> javadoc situation, which allows him to have a clear view on the
> consequences of his own actions. It is a fairly puristic stand point,
> but I appreciate that he feels that he needs it to do a good
> job. After all, his is quite a far-reaching code change.
>
> We do need to retain a few deprecated methods, as they are deprecated
> parts of the public API, of which you are one of the users. It would
> be good if we could document the new alternatives, and fix a time when
> they can be removed. Perhaps at release 1.1, as they were already
> deprecated at release 1.0?
>
> We will see how the other team members stand in this issue, but I will
> be thoroughly disappointed if we cannot move forward efficiently with
> this work.
>
> Simon
>
> On Fri, Aug 13, 2010 at 04:47:07PM +0200, Jeremias Maerki wrote:
> > I've mentioned the deprecated methods in my reply to Vincent. I'll
> > restore two instances which can be handled later. At least one is kind
> > of important as long as I haven't done a Barcode4J 2.1 release (which is
> > long overdue).
> >
> > Do I understand you correctly, Simon, that you're OK to leave the CS
> > comments for now and revisit later? I could live with that, too. If I
> > read Vincent and Chris correctly, they are not absolutely against having
> > the CS comments for now although they are not at all happy about them.
> > Please correct me if I got that wrong! Removing them later is always a
> > possibility. I'm not too happy to disregard a majority opinion
> > especially since Glenn is not yet a committer. But I guess leaving the
> > CS comments for now allows us to continue and we can still reduce (or
> > get rid of) the CS comments later.
> >
> > I know I'm currently behaving like a flag in the wind but I'm really a
> > bit clueless what the best way is since we do not have a consensus right
> > now. But I'd like to continue here as quickly as possible. I didn't get
> > to handle the patch today due to a support request (FOP go boom with PDF
> > sizes over 2GB). But the weather doesn't look to good here during the
> > weekend so I may be able to get this done tomorrow.
> >
> > On 13.08.2010 14:40:55 Simon Pepping wrote:
> > > Glenn,
> > >
> > > On Fri, Aug 13, 2010 at 05:07:52PM +0800, Glenn Adams wrote:
> > > > In any case, we now appear to be at a juncture where one of the following
> > > > options may be implemented:
> > > >
> > > > (1) leave the CS* comments in place, but DON'T change the checkstyle rules
> > > > AT THIS TIME (but reserve option to change later)
> > > > (2) remove the CS* comments, but DON'T change the checkstyle rules, leaving
> > > > at least 279 warnings/errors to be produced;
> > > > (3) remove the CS* comments, but DO change the checkstyle rules AT THIS TIME
> > > > such that none of the CS* comments are required
> > > >
> > > > I prefer option #1.
> > > >
> > > > I cannot accept option #2, since it leaves a large number of reported
> > > > warnings, thus negating my primary goal in creating this patch.
> > > >
> > > > I can live with option #3, although it requires editing around 100 files to
> > > > remove the CS* comments. And it also requires modifying the checkstyle rule
> > > > set, and in some cases removing or weakening potentially useful rules.
> > >
> > > I would prefer something like option #2, and so do a few other
> > > committers. I understand this produces an unacceptable working mode
> > > for you. I can live with that, and we can review the CHECKSTYLE
> > > comments later in an effort to make further improvements.
> > >
> > > I would like to hear Jeremias' comment on the removal of the
> > > deprecated methods. Deprecated methods are a fact of life.
>
> --
> Simon Pepping
> home page: http://www.leverkruid.eu
Jeremias Maerki
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc
warnings (a proposal for next steps)
Posted by Chris Bowditch <bo...@hotmail.com>.
Jeremias Maerki wrote:
Hi All,
> I've mentioned the deprecated methods in my reply to Vincent. I'll
> restore two instances which can be handled later. At least one is kind
> of important as long as I haven't done a Barcode4J 2.1 release (which is
> long overdue).
>
> Do I understand you correctly, Simon, that you're OK to leave the CS
> comments for now and revisit later? I could live with that, too. If I
> read Vincent and Chris correctly, they are not absolutely against having
> the CS comments for now although they are not at all happy about them.
> Please correct me if I got that wrong! Removing them later is always a
> possibility. I'm not too happy to disregard a majority opinion
> especially since Glenn is not yet a committer. But I guess leaving the
> CS comments for now allows us to continue and we can still reduce (or
> get rid of) the CS comments later.
I think you understood me correctly. Whilst I prefer not to have lots of
CS comments scattered around I also don't want to stop Glenn working. My
preferred option from the list of 3 is #2, but as Glenn already
indicated that is unacceptable to him I'm happy to vote -0 here.
>
> I know I'm currently behaving like a flag in the wind but I'm really a
> bit clueless what the best way is since we do not have a consensus right
> now. But I'd like to continue here as quickly as possible. I didn't get
> to handle the patch today due to a support request (FOP go boom with PDF
> sizes over 2GB). But the weather doesn't look to good here during the
> weekend so I may be able to get this done tomorrow.
Thanks,
Chris
>
> On 13.08.2010 14:40:55 Simon Pepping wrote:
>> Glenn,
>>
>> On Fri, Aug 13, 2010 at 05:07:52PM +0800, Glenn Adams wrote:
>>> In any case, we now appear to be at a juncture where one of the following
>>> options may be implemented:
>>>
>>> (1) leave the CS* comments in place, but DON'T change the checkstyle rules
>>> AT THIS TIME (but reserve option to change later)
>>> (2) remove the CS* comments, but DON'T change the checkstyle rules, leaving
>>> at least 279 warnings/errors to be produced;
>>> (3) remove the CS* comments, but DO change the checkstyle rules AT THIS TIME
>>> such that none of the CS* comments are required
>>>
>>> I prefer option #1.
>>>
>>> I cannot accept option #2, since it leaves a large number of reported
>>> warnings, thus negating my primary goal in creating this patch.
>>>
>>> I can live with option #3, although it requires editing around 100 files to
>>> remove the CS* comments. And it also requires modifying the checkstyle rule
>>> set, and in some cases removing or weakening potentially useful rules.
>> I would prefer something like option #2, and so do a few other
>> committers. I understand this produces an unacceptable working mode
>> for you. I can live with that, and we can review the CHECKSTYLE
>> comments later in an effort to make further improvements.
>>
>> I would like to hear Jeremias' comment on the removal of the
>> deprecated methods. Deprecated methods are a fact of life.
>>
>> Simon
>>
>> --
>> Simon Pepping
>> home page: http://www.leverkruid.eu
>
>
>
>
> Jeremias Maerki
>
>
>
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc
warnings (a proposal for next steps)
Posted by Simon Pepping <sp...@leverkruid.eu>.
I want to move forward with Glenn's work. As you wrote, it is an
important addition to FOP which we cannot let go unused.
I feel that the CHECKSTYLE comments are clear, and allow us to take
any action later that we require. They could be harmful if we would
feel that further work would have to be done, but actually is not
done. Then the comments will hide that situation. But that is up to
us.
I understand that Glenn wants to work from a clean checkstyle and
javadoc situation, which allows him to have a clear view on the
consequences of his own actions. It is a fairly puristic stand point,
but I appreciate that he feels that he needs it to do a good
job. After all, his is quite a far-reaching code change.
We do need to retain a few deprecated methods, as they are deprecated
parts of the public API, of which you are one of the users. It would
be good if we could document the new alternatives, and fix a time when
they can be removed. Perhaps at release 1.1, as they were already
deprecated at release 1.0?
We will see how the other team members stand in this issue, but I will
be thoroughly disappointed if we cannot move forward efficiently with
this work.
Simon
On Fri, Aug 13, 2010 at 04:47:07PM +0200, Jeremias Maerki wrote:
> I've mentioned the deprecated methods in my reply to Vincent. I'll
> restore two instances which can be handled later. At least one is kind
> of important as long as I haven't done a Barcode4J 2.1 release (which is
> long overdue).
>
> Do I understand you correctly, Simon, that you're OK to leave the CS
> comments for now and revisit later? I could live with that, too. If I
> read Vincent and Chris correctly, they are not absolutely against having
> the CS comments for now although they are not at all happy about them.
> Please correct me if I got that wrong! Removing them later is always a
> possibility. I'm not too happy to disregard a majority opinion
> especially since Glenn is not yet a committer. But I guess leaving the
> CS comments for now allows us to continue and we can still reduce (or
> get rid of) the CS comments later.
>
> I know I'm currently behaving like a flag in the wind but I'm really a
> bit clueless what the best way is since we do not have a consensus right
> now. But I'd like to continue here as quickly as possible. I didn't get
> to handle the patch today due to a support request (FOP go boom with PDF
> sizes over 2GB). But the weather doesn't look to good here during the
> weekend so I may be able to get this done tomorrow.
>
> On 13.08.2010 14:40:55 Simon Pepping wrote:
> > Glenn,
> >
> > On Fri, Aug 13, 2010 at 05:07:52PM +0800, Glenn Adams wrote:
> > > In any case, we now appear to be at a juncture where one of the following
> > > options may be implemented:
> > >
> > > (1) leave the CS* comments in place, but DON'T change the checkstyle rules
> > > AT THIS TIME (but reserve option to change later)
> > > (2) remove the CS* comments, but DON'T change the checkstyle rules, leaving
> > > at least 279 warnings/errors to be produced;
> > > (3) remove the CS* comments, but DO change the checkstyle rules AT THIS TIME
> > > such that none of the CS* comments are required
> > >
> > > I prefer option #1.
> > >
> > > I cannot accept option #2, since it leaves a large number of reported
> > > warnings, thus negating my primary goal in creating this patch.
> > >
> > > I can live with option #3, although it requires editing around 100 files to
> > > remove the CS* comments. And it also requires modifying the checkstyle rule
> > > set, and in some cases removing or weakening potentially useful rules.
> >
> > I would prefer something like option #2, and so do a few other
> > committers. I understand this produces an unacceptable working mode
> > for you. I can live with that, and we can review the CHECKSTYLE
> > comments later in an effort to make further improvements.
> >
> > I would like to hear Jeremias' comment on the removal of the
> > deprecated methods. Deprecated methods are a fact of life.
--
Simon Pepping
home page: http://www.leverkruid.eu
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
I've mentioned the deprecated methods in my reply to Vincent. I'll
restore two instances which can be handled later. At least one is kind
of important as long as I haven't done a Barcode4J 2.1 release (which is
long overdue).
Do I understand you correctly, Simon, that you're OK to leave the CS
comments for now and revisit later? I could live with that, too. If I
read Vincent and Chris correctly, they are not absolutely against having
the CS comments for now although they are not at all happy about them.
Please correct me if I got that wrong! Removing them later is always a
possibility. I'm not too happy to disregard a majority opinion
especially since Glenn is not yet a committer. But I guess leaving the
CS comments for now allows us to continue and we can still reduce (or
get rid of) the CS comments later.
I know I'm currently behaving like a flag in the wind but I'm really a
bit clueless what the best way is since we do not have a consensus right
now. But I'd like to continue here as quickly as possible. I didn't get
to handle the patch today due to a support request (FOP go boom with PDF
sizes over 2GB). But the weather doesn't look to good here during the
weekend so I may be able to get this done tomorrow.
On 13.08.2010 14:40:55 Simon Pepping wrote:
> Glenn,
>
> On Fri, Aug 13, 2010 at 05:07:52PM +0800, Glenn Adams wrote:
> > In any case, we now appear to be at a juncture where one of the following
> > options may be implemented:
> >
> > (1) leave the CS* comments in place, but DON'T change the checkstyle rules
> > AT THIS TIME (but reserve option to change later)
> > (2) remove the CS* comments, but DON'T change the checkstyle rules, leaving
> > at least 279 warnings/errors to be produced;
> > (3) remove the CS* comments, but DO change the checkstyle rules AT THIS TIME
> > such that none of the CS* comments are required
> >
> > I prefer option #1.
> >
> > I cannot accept option #2, since it leaves a large number of reported
> > warnings, thus negating my primary goal in creating this patch.
> >
> > I can live with option #3, although it requires editing around 100 files to
> > remove the CS* comments. And it also requires modifying the checkstyle rule
> > set, and in some cases removing or weakening potentially useful rules.
>
> I would prefer something like option #2, and so do a few other
> committers. I understand this produces an unacceptable working mode
> for you. I can live with that, and we can review the CHECKSTYLE
> comments later in an effort to make further improvements.
>
> I would like to hear Jeremias' comment on the removal of the
> deprecated methods. Deprecated methods are a fact of life.
>
> Simon
>
> --
> Simon Pepping
> home page: http://www.leverkruid.eu
Jeremias Maerki
Re: AW: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
Thanks for the idea. But I'm not sure if that creates too much fuzz when
merging changes. What do the others think?
On 13.08.2010 14:47:42 Georg Datterl wrote:
> Hi,
>
> What if the CS* comments are removed in trunk, so the committers are
> happy, but accepted in the branch, so Glenn can work as he wants to?
> Not a perfect solution, but maybe an acceptable compromise, as long as
> somebody removes the comments prior to merging the branch back into
> trunk?
>
> Regards,
>
> Georg Datterl
<snip/>
Jeremias Maerki
AW: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc
warnings (a proposal for next steps)
Posted by Georg Datterl <ge...@geneon.de>.
Hi,
What if the CS* comments are removed in trunk, so the committers are happy, but accepted in the branch, so Glenn can work as he wants to? Not a perfect solution, but maybe an acceptable compromise, as long as somebody removes the comments prior to merging the branch back into trunk?
Regards,
Georg Datterl
------ Kontakt ------
Georg Datterl
Geneon media solutions gmbh
Gutenstetter Straße 8a
90449 Nürnberg
HRB Nürnberg: 17193
Geschäftsführer: Yong-Harry Steiert
Tel.: 0911/36 78 88 - 26
Fax: 0911/36 78 88 - 20
www.geneon.de
Weitere Mitglieder der Willmy MediaGroup:
IRS Integrated Realization Services GmbH: www.irs-nbg.de
Willmy PrintMedia GmbH: www.willmy.de
Willmy Consult & Content GmbH: www.willmycc.de
-----Ursprüngliche Nachricht-----
Von: Simon Pepping [mailto:spepping@leverkruid.eu]
Gesendet: Freitag, 13. August 2010 14:41
An: fop-dev@xmlgraphics.apache.org
Betreff: Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
Glenn,
On Fri, Aug 13, 2010 at 05:07:52PM +0800, Glenn Adams wrote:
> In any case, we now appear to be at a juncture where one of the following
> options may be implemented:
>
> (1) leave the CS* comments in place, but DON'T change the checkstyle rules
> AT THIS TIME (but reserve option to change later)
> (2) remove the CS* comments, but DON'T change the checkstyle rules, leaving
> at least 279 warnings/errors to be produced;
> (3) remove the CS* comments, but DO change the checkstyle rules AT THIS TIME
> such that none of the CS* comments are required
>
> I prefer option #1.
>
> I cannot accept option #2, since it leaves a large number of reported
> warnings, thus negating my primary goal in creating this patch.
>
> I can live with option #3, although it requires editing around 100 files to
> remove the CS* comments. And it also requires modifying the checkstyle rule
> set, and in some cases removing or weakening potentially useful rules.
I would prefer something like option #2, and so do a few other
committers. I understand this produces an unacceptable working mode
for you. I can live with that, and we can review the CHECKSTYLE
comments later in an effort to make further improvements.
I would like to hear Jeremias' comment on the removal of the
deprecated methods. Deprecated methods are a fact of life.
Simon
--
Simon Pepping
home page: http://www.leverkruid.eu
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc
warnings (a proposal for next steps)
Posted by Simon Pepping <sp...@leverkruid.eu>.
Glenn,
On Fri, Aug 13, 2010 at 05:07:52PM +0800, Glenn Adams wrote:
> In any case, we now appear to be at a juncture where one of the following
> options may be implemented:
>
> (1) leave the CS* comments in place, but DON'T change the checkstyle rules
> AT THIS TIME (but reserve option to change later)
> (2) remove the CS* comments, but DON'T change the checkstyle rules, leaving
> at least 279 warnings/errors to be produced;
> (3) remove the CS* comments, but DO change the checkstyle rules AT THIS TIME
> such that none of the CS* comments are required
>
> I prefer option #1.
>
> I cannot accept option #2, since it leaves a large number of reported
> warnings, thus negating my primary goal in creating this patch.
>
> I can live with option #3, although it requires editing around 100 files to
> remove the CS* comments. And it also requires modifying the checkstyle rule
> set, and in some cases removing or weakening potentially useful rules.
I would prefer something like option #2, and so do a few other
committers. I understand this produces an unacceptable working mode
for you. I can live with that, and we can review the CHECKSTYLE
comments later in an effort to make further improvements.
I would like to hear Jeremias' comment on the removal of the
deprecated methods. Deprecated methods are a fact of life.
Simon
--
Simon Pepping
home page: http://www.leverkruid.eu
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc
warnings (a proposal for next steps)
Posted by Glenn Adams <gl...@skynav.com>.
On Fri, Aug 13, 2010 at 4:32 PM, Simon Pepping <sp...@leverkruid.eu>wrote:
>
> Glenn notes that he used comments to suppress checkstyle warnings in
> such cases as:
>
> - certain uses of package, protected, or public visibility of fields,
> which would have required a fairly large number of changes to substitute
> calls to new getX() or setX(...) methods;
>
> Leaving the warnings would be a sign that some work is to be
> done. Suppressing the warnings gives the false idea that no more work
> is to be done, and that all comments represent a sound judgment to
> leave the code intentionally as is.
>
> Otherwise I am in favour of using warnings to mark code that
> intentionally does not comply with the rules, at the judgment of the
> developer.
The primary reason I undertook this cleanup work is because my complex
scripts patch touched so many core files that already contained warnings. In
modifying them, I wished to ensure that I did not introduce new warnings,
however determining this was quite painful and time consuming given the
>2000 warnings that existed.
In this state, it is very easy to allow new, unintentional errors to creep
in, or to ignore this form of testing altogether. I think this has been the
status quo, and is probably responsible for the existence of so many
warnings.
On the other hand, if there are no warnings during build, javadoc build,
checkstyle, etc., then it is easy for a developer to notice and fix problems
before they become unmanageable. That was my goal in this patch, to create a
noiseless baseline.
As the process proceeded, I found I could address the majority of
warnings/errors directly, without resorting to warning suppression. However,
as I've already pointed out, this was not always possible or would have been
impractical or potentially destabilizing to implement the necessary changes.
In these cases, inline suppressions did the trick. Further, their existence
left in place an easy mechanism for learning of, tracking, and subsequently
applying more in-depth fixes. A mere grep of the code easily shows where the
were used, and, in fact, it would be easy enough to add a build target that
produces a report of their presence.
Furthermore, I did not want to undertake at this time a discussion
(argument?) about what is good style or bad style, what should be always
enforced or what should be merely a guideline. I continue to be reluctant to
have such a discussion, partly because I think it is a waste of time that
could be applied to other more useful work.
In any case, we now appear to be at a juncture where one of the following
options may be implemented:
(1) leave the CS* comments in place, but DON'T change the checkstyle rules
AT THIS TIME (but reserve option to change later)
(2) remove the CS* comments, but DON'T change the checkstyle rules, leaving
at least 279 warnings/errors to be produced;
(3) remove the CS* comments, but DO change the checkstyle rules AT THIS TIME
such that none of the CS* comments are required
I prefer option #1.
I cannot accept option #2, since it leaves a large number of reported
warnings, thus negating my primary goal in creating this patch.
I can live with option #3, although it requires editing around 100 files to
remove the CS* comments. And it also requires modifying the checkstyle rule
set, and in some cases removing or weakening potentially useful rules.
G.
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc
warnings (a proposal for next steps)
Posted by Simon Pepping <sp...@leverkruid.eu>.
On Thu, Aug 12, 2010 at 02:25:33PM +0200, Jeremias Maerki wrote:
Hi,
I have returned and read the discussions. I have the following
remarks:
Building fop with jdk 1.4, as required, gives an error when
checkstyle-all-5.0.jar is present. The major.minor version 49.0 is not
supported. Thus removing checkstyle-4.0 gives an inconsistency between
the development environment and the release build environment, which
is workable but at the same time annoying.
Some methods marked deprecated were part of our (unofficial)
API. Deprecation requires some time in which application builders can
change over. Indeed, a description of the alternative and a time frame
to change over would have been useful. If we remove these methods, we
must be prepared to face application builders at our next release.
Glenn notes that he used comments to suppress checkstyle warnings in
such cases as:
- certain uses of package, protected, or public visibility of fields,
which would have required a fairly large number of changes to substitute
calls to new getX() or setX(...) methods;
Leaving the warnings would be a sign that some work is to be
done. Suppressing the warnings gives the false idea that no more work
is to be done, and that all comments represent a sound judgment to
leave the code intentionally as is.
Otherwise I am in favour of using warnings to mark code that
intentionally does not comply with the rules, at the judgment of the
developer.
> I would suggest the following as our next steps:
>
> 1. Clarify the thing with LineBreak*.
> 2. Decide (quickly, please) whether to remove the //CS comments or to
> allow them for now and optionally do something about them later. (I'm
> tending towards removing them but I don't have a problem if we do it the
> other way.)
> 3. Commit the patch to Trunk more or less as is (pending //CS decision).
> 3. Adjust the Checkstyle profile to allow "log" and disallow whitespace
> before and after parantheses. Then remove "log"-related //CS constants
> and excessive whitespace.
> 4. Merge the changes into the Temp_ComplexScripts parts.
> 5. Glenn could then provide a new patch against the branch which we
> could do a cursory review on. We apply that and experiment with what
> he's built. He can continue his work.
> 6. We continue to incrementally improve our coding standards.
>
> I'm happy to do the grunt work. Like Glenn, I don't like to hold
> principle discussions right now because that holds up several people
> from doing day-to-day work. That doesn't mean we can't hold them, but I
> don't see why we have to do it as a precondition to processing this
> patch. The patch gets us further but doesn't preclude any futher
> improvements later.
>
> Please, let's get this done.
Generally I agree with this plan. Specifically, I do not want to wait
for future discussions about better rules. That would make better the
enemy of good. I want to take the practical approach that the current
work is an improvement over what we had, and must be applied after
concensus over the above discussed points.
Simon
--
Simon Pepping
home page: http://www.leverkruid.eu