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. >>> > > > >>> > > > >>> > > > >>> > > > >>> > > > >>> > > > >>> > > > >>> > > >>> > >>> >>
