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> 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:_e(%7B%7D,'cvml','jacq...@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.dunn...@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 <ebeg...@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 <ebeg...@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.
>> > > >
>> > > >
>> > > >
>> > > >
>> > > >
>> > > >
>> > > >
>> > >
>> >
>>
>
