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]> 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] > <javascript:_e(%7B%7D,'cvml','[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. >> > > > >> > > > >> > > > >> > > > >> > > > >> > > > >> > > > >> > > >> > >> >
