Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings
On Thu, Aug 12, 2010 at 3:18 PM, Glenn Adams gl...@skynav.com wrote: that is a reasonable objection to removing the file, so indeed i would not object to leaving it in place; however, i did not test checkstyle 1.4, and indeed, there are a few changes that appear at the end of the new checkstyles-5.1.xml file which would need to be added to the older checkstyle-1.4.xml file (presuming they are supported in 1.4) in order to enable the currently used suppressions; on the other hand, i'm not sure we want to make (or continue to make) FOP compatible with specific IDEs, since at present, only the command line ANT build is effectively supported; but in the interest of compromise, i won't object to retaining the old 1.4 file (you may wish to update it with the new suppression clauses), namely, by adding: s/1.4/4.0/g
DO NOT REPLY [Bug 1063] fop does not handle large fo files
https://issues.apache.org/bugzilla/show_bug.cgi?id=1063 Antti Karanta antti.kara...@napa.fi changed: What|Removed |Added CC||antti.kara...@napa.fi -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings
vincent, my apologies if I offended, as that was not my intent; i'm sure we will manage to work our way through this towards a successful conclusion; respectfully, glenn On Thu, Aug 12, 2010 at 7:41 PM, Vincent Hennebert vhenneb...@gmail.comwrote: This message lacks of courtesy, therefore I do not wish to continue the discussion.
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
DO NOT REPLY [Bug 1063] fop does not handle large fo files
https://issues.apache.org/bugzilla/show_bug.cgi?id=1063 --- Comment #18 from M.H. mhilp...@gmx.de 2010-08-12 08:27:58 EDT --- Even with FOP 0.95, we sometimes have OutOfMemory issues with larger FO files (or FO files producing large PDF files with several dozen or hundred pages. We tweaked our main server application to use less memoryand can throw as much memory as possible to FOP, but I guess FOP processing is still memory bound. I hoped to have some kind of streaming processing of XSL-FO just like the SAX XML processor. -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
Re: classpath
I agree that we should remove this. The manifest's Classpath is quite restricted in its usefulness. Furthermore, I've long ago added some code that builds up the classpath dynamically when necessary. Hardly anyone will call FOP by java -jar fop.jar. Most people will use one of the scripts (I do, too). If noone objects, I'll remove it. On 10.08.2010 16:48:18 Eric Douglas wrote: I see there's still a classpath statement in the 1.0 Manifest.mf file. Is this necessary? Could this be removed in the next version, and document a classpath parameter to pass for manually executing? I run this with embedded code from an app which already has a classpath, so I have to recompile as the manifest classpath interferes, and having this classpath hardcoded prevents testing with updated versions of the referenced jars. Jeremias Maerki
Re: Build errors
linkmap.xml? I don't think we have a file with that name in FOP. Could that be coming from Apache Forrest somehow, maybe due to a buggy XML parser maybe? Maybe putting a current Xerces and Xalan in the JRE's lib/endorsed directory may change something. Otherwise, please provide a snippet from the output log. On 10.08.2010 17:27:00 Eric Douglas wrote: When I download the source for fop 1.0, the ant build shows successful, but if I try a regular build just to check for errors before running the ant build I get a bunch of error messages such as the content of element type li must match... (on linkmap.xml). Is this normal or am I missing something? Jeremias Maerki
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
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
Re: [Bug 49733] [PATCH] resolve compilation, checkstyle, javadoc warnings (a proposal for next steps)
inline
On Thu, Aug 12, 2010 at 8:25 PM, Jeremias Maerki d...@jeremias-maerki.chwrote:
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)
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.
