drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Edmon Begoli <ebeg...@gmail.com>
Subject Re: Maven build failing on checkstyle
Date Thu, 10 Sep 2015 09:46:31 GMT
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 <jacques@dremio.com> 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.dunning@gmail.com> 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 <ebegoli@gmail.com> 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 <ebegoli@gmail.com>
> 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.
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message