Hey Edmon, I completely agree that Checkstyle can be a pain. I've worked on a couple projects where the rules are truly draconian. In Drill today, I think we have only the following rules:
1) No trailing whitespace 2) No If statements without brackets (other than ternary) 3) No imports of the wrong guava classes (there are two other versions that are on the classpath inside of other packages) So the check you hit actually has nothing to do with Javadocs. As far as I know, we have no requirements for Javadocs. I'm generally fine removing the trailing white space requirement specifically from Javadocs (but I'm not sure how easy that is in the Checkstyle plugin). I think that the trailing whitespace check does provide great use in code so I'd prefer to keep it. What do you think? -- Jacques Nadeau CTO and Co-Founder, Dremio On Thu, Sep 10, 2015 at 4:39 AM, Edmon Begoli <[email protected]> wrote: > My proposal: > > 1) Loosen checks between 1.2 and 1.3 releases > > 2) let me and as many other people as they are willing to contribute effort > to add proper javadoc. > Ask any contributor touching methods to add few lines of missing javadoc > > 3) once done tighten up the build to the max enforcing all best styling > practices including javadoc on all public methods and classes. > > On Thursday, September 10, 2015, Edmon Begoli <[email protected]> wrote: > > > Long story short (sorry :-|) - does it make sense to have a build > > stopping check for \s+$ in a javadoc and not check and stop the build for > > missing or improper javadoc? > > > > On Thursday, September 10, 2015, Edmon Begoli <[email protected] > > <javascript:_e(%7B%7D,'cvml','[email protected]');>> wrote: > > > >> My observation on this, as a newcomer, is that it is a paradoxical for a > >> build to fail on extra space in a meaningful javadoc, but not on the > >> actual missing text, @param, @see or a @return sections on the public > >> methods. > >> > >> On many classes there is no javadoc at all. On some, it is invalid. > >> > >> This makes it very hard for a contributor to get started because some of > >> these classes are components of a pattern in deep interface > implementation > >> and inheritance hierarchy where you need to know exactly what two or > three > >> methods are supposed to do in order to implement them. > >> > >> Examples: > >> > >> // New text reader, complies with the RFC 4180 standard for text/csv > files > >> public class CompliantTextRecordReader extends AbstractRecordReader { > >> > >> // checks to see if we are querying all columns(star) or individual > columns > >> @Override > >> public boolean isStarQuery() { > >> > >> > >> While I strongly believe in a self-commenting code, it would make it > >> million times easier if there was a some kind of enforcement either in > form > >> of a human code review on the pull request, or even a checkstyle that > >> requires, for example, a minimum of 20 words on a class/interface > javadoc > >> and a minimum of 10 on a public method. (Regex for the presence of > >> alphanumeric tokens or English words in a javadoc) > >> > >> Also, I *volonteer* to just write a javadoc for beginning, but I think > >> it needs to be there. Drill is just way too complex and way too > >> undocumented for an easy start for a contributor. > >> > >> Here is an example from POI for how to use the API. If you notice, a > >> javadoc is 5x the code because it is an intro stuff showing how to use > it: > >> > >> > http://svn.apache.org/repos/asf/poi/trunk/src/examples/src/org/apache/poi/ss/examples/ToCSV.java > >> > >> or, an example from Hive of some the core classes: > >> > >> > https://github.com/apache/hive/blob/master/storage-api/src/java/org/apache/hadoop/hive/ql/exec/vector/BytesColumnVector.java > >> > >> Thoughts? > >> > >> > >> On Thursday, September 10, 2015, Jacques Nadeau <[email protected]> > >> wrote: > >> > >>> Hey Ted, > >>> > >>> FYI, we added trailing spaces check on purpose. Please open a > discussion > >>> rather than making a random decision. If anything our checkstyle is far > >>> too > >>> lenient which has led to poor consistency and missing comments. > >>> On Sep 9, 2015 11:18 AM, "Ted Dunning" <[email protected]> wrote: > >>> > >>> > Checkstyle is clearly being too picky here. > >>> > > >>> > The only problem with spaces at the end of a line is that some tools > >>> strip > >>> > them out automagically. This leads to format changes that make > reviews > >>> > (very slightly) more difficult. > >>> > > >>> > I would be willing to fix the checkstyle profile to be less draconian > >>> if > >>> > you would be willing to file the JIRA. > >>> > > >>> > > >>> > > >>> > On Wed, Sep 9, 2015 at 5:14 AM, Edmon Begoli <[email protected]> > >>> wrote: > >>> > > >>> > > and I am sorry to bug you with this but to me, this was a prefectly > >>> > > formatted javadoc and I was surprised to see build failing on it: > >>> > > > >>> > > /** Abstract class for StorePlugin implementations. > >>> > > * See StoragePlugin for description of the interface intent and > its > >>> > > methods. > >>> > > */ > >>> > > public abstract class AbstractStoragePlugin implements > StoragePlugin{ > >>> > > static final org.slf4j.Logger logger = > >>> > > org.slf4j.LoggerFactory.getLogger(AbstractStoragePlugin.class); > >>> > > > >>> > > However, it had a space before the end of the line first line, and > >>> > > checkstyle did not like it. I was using vim, not IDE. > >>> > > > >>> > > I am switching to IDEA ... > >>> > > > >>> > > > >>> > > On Tue, Sep 8, 2015 at 11:48 PM, Edmon Begoli <[email protected]> > >>> wrote: > >>> > > > >>> > > > I am running build on my fork, and Maven build is failing on the > >>> > > > checkstyle: > >>> > > > > >>> > > > excerpt ... > >>> > > > > >>> > > > [INFO] --- maven-checkstyle-plugin:2.12.1:check > >>> > (checkstyle-validation) @ > >>> > > > drill-java-exec --- > >>> > > > > >>> > > > [INFO] Starting audit... > >>> > > > > >>> > > > > >>> > > > >>> > > >>> > /Users/ebegoli/drill/exec/java-exec/src/main/java/org/apache/drill/exec/store/AbstractStoragePlugin.java:31: > >>> > > > Line matches the illegal pattern '\s+$'. > >>> > > > > >>> > > > > >>> > > > >>> > > >>> > /Users/ebegoli/drill/exec/java-exec/src/main/java/org/apache/drill/exec/store/AbstractStoragePlugin.java:33: > >>> > > > Line matches the illegal pattern '\s+$'. > >>> > > > > >>> > > > > >>> > > > >>> > > >>> > /Users/ebegoli/drill/exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/easy/EasyFormatPlugin.java:118: > >>> > > > Line matches the illegal pattern '\s+$'. > >>> > > > > >>> > > > > >>> > > > >>> > > >>> > /Users/ebegoli/drill/exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePlugin.java:30: > >>> > > > Line matches the illegal pattern '\s+$'. > >>> > > > > >>> > > > > >>> > > > >>> > > >>> > /Users/ebegoli/drill/exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePlugin.java:35: > >>> > > > Line matches the illegal pattern '\s+$'. > >>> > > > > >>> > > > > >>> > > > >>> > > >>> > /Users/ebegoli/drill/exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePlugin.java:44: > >>> > > > Line matches the illegal pattern '\s+$'. > >>> > > > > >>> > > > > >>> > > > >>> > > >>> > /Users/ebegoli/drill/exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePlugin.java:45: > >>> > > > Line matches the illegal pattern '\s+$'. > >>> > > > > >>> > > > > >>> > > > >>> > > >>> > /Users/ebegoli/drill/exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePlugin.java:71: > >>> > > > Line matches the illegal pattern '\s+$'. > >>> > > > > >>> > > > > >>> > > > >>> > > >>> > /Users/ebegoli/drill/exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePlugin.java:74: > >>> > > > Line matches the illegal pattern '\s+$'. > >>> > > > > >>> > > > Audit done. > >>> > > > > >>> > > > It looks like Javadoc checkstyle if failing. These are included > in > >>> my > >>> > > pull: > >>> > > > > >>> > > > https://github.com/apache/drill/pull/139 > >>> > > > > >>> > > > > >>> > > > Can someone please advise how do I and should I either suppress > >>> these > >>> > or > >>> > > > fix the issue. > >>> > > > > >>> > > > It is a properly structured javadoc. Starts with /** and ends > with > >>> */. > >>> > > > > >>> > > > Not sure what else is required, but I will happy to fix it to > make > >>> it > >>> > > pass > >>> > > > the checkstyle. > >>> > > > > >>> > > > > >>> > > > > >>> > > > > >>> > > > > >>> > > > > >>> > > > > >>> > > > >>> > > >>> > >> >
