My opinion is to keep trailing spaces checks in as they are. On Thursday, September 10, 2015, Jacques Nadeau <jacq...@dremio.com> wrote:
> 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 <ebeg...@gmail.com > <javascript:;>> 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 <ebeg...@gmail.com > <javascript:;>> 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 <ebeg...@gmail.com > <javascript:;> > > > <javascript:_e(%7B%7D,'cvml','ebeg...@gmail.com <javascript:;>');>> > 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 <jacq...@dremio.com > <javascript:;>> > > >> 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" <ted.dunn...@gmail.com > <javascript:;>> 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 <ebeg...@gmail.com > <javascript:;>> > > >>> 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 <ebeg...@gmail.com > <javascript:;>> > > >>> 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. > > >>> > > > > > >>> > > > > > >>> > > > > > >>> > > > > > >>> > > > > > >>> > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >> > > >