Mailing-List: contact dev-help@drill.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@drill.apache.org
MIME-Version: 1.0
In-Reply-To: 
 <CAJrw0ORQg2PyTaF4uDxDDcXXNL6eTb15W-5dHGh9Rrkq_USwHQ@mail.gmail.com>
References: 
 <CAGj+Ysc9uruVSqzc_0T86GAXA+HjCqeJN84kTP0m8bN0WcWGyg@mail.gmail.com>
	<CAGj+YseVDvC30UrE-FfKynD98sEr52+JpvPL+MJh1L6ZBUWu5g@mail.gmail.com>
	<CAJwFCa3XFDhzNPmNa8dQW+Gj1Z_1cfCsTiktCii81L+52ReG_g@mail.gmail.com>
	<CAJrw0OTJoTjX4+qzBm-7P4sRyFm0ipK74cf8nj5MbV1LpgJfAg@mail.gmail.com>
	<CAGj+Ysd76rN4UNKM_=ZHXWTDAM16Lmc88uVOJwG7w9S2rJ2Pkw@mail.gmail.com>
	<CAGj+Yscv9XhpueuGSZ+Q8yN7hvEY_McSeq6B5JPgYnN8PRC9zA@mail.gmail.com>
	<CAGj+YscOgokT8XvY2EbsgB53iYoBoueRQYE3U1Z=8yyMmHOATQ@mail.gmail.com>
	<CAJrw0ORQg2PyTaF4uDxDDcXXNL6eTb15W-5dHGh9Rrkq_USwHQ@mail.gmail.com>
Date: Thu, 10 Sep 2015 07:05:47 -0700
Message-ID: 
 <CAMGsNzAjTNJFjW9A3yRT9Y-484fnVYAUCFFedssVd2gHz4LWng@mail.gmail.com>
Subject: Re: Maven build failing on checkstyle
From: Chris Westin <chriswestin42@gmail.com>
To: dev@drill.apache.org
Content-Type: multipart/alternative; boundary=001a11c3f5be85d4a7051f65182e

--001a11c3f5be85d4a7051f65182e
Content-Type: text/plain; charset=UTF-8

Since most editors and IDEs will strip trailing whitespace everywhere in a
file (code or comments), we should leave it in to avoid getting spurious
diffs.

On Thu, Sep 10, 2015 at 7:02 AM, Jacques Nadeau <jacques@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 <ebegoli@gmail.com> 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 <ebegoli@gmail.com> 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 <ebegoli@gmail.com
> > > <javascript:_e(%7B%7D,'cvml','ebegoli@gmail.com');>> 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 <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.
> > >>> > > >
> > >>> > > >
> > >>> > > >
> > >>> > > >
> > >>> > > >
> > >>> > > >
> > >>> > > >
> > >>> > >
> > >>> >
> > >>>
> > >>
> >
>

--001a11c3f5be85d4a7051f65182e--