Let's not muddle the original suggestion that Tim had which was simply
about having checkstyle do the checks for unused imports. That seems
reasonable to me. A few other checkstyle suggestions that people have made
could also be added.
All other discussion about code structure, unit tests etc should be
discussed in a separate thread. IMHO it is easy to be critical of existing
structure, much harder when starting from scratch under time constraints.
-Aman
On Mon, Sep 11, 2017 at 9:53 AM, Paul Rogers <prog...@mapr.com> wrote:
> The check style improvements are good, they will likely save developers
> minutes per day.
>
> I would encourage us to consider other time savers.
>
> In normal development builds, Checkstyle issues should be warnings, not
> errors. Developers waste many five-minute build cycles per day when we do a
> quick & dirty prototype, only to wait for the build and have it fail due to
> trivial check-style errors. Checkstyle should be a finishing touch:
> enforced only prior to commit, but not during normal development. Issuing
> warnings, rather than errors, will allow developers to proceed with the
> test under way, and fix the style issues for the next private build.
>
> Code structure issues waste hours per day. The lack of high-quality unit
> tests means that Drill code has many uncaught bugs. Those slow development
> of new work as we slam into them again and again. Drill code is poorly
> structured and undocumented, meaning that the simplest change is very
> expensive.
>
> Worse, the lack of true unit tests has become the Drill standard and thus
> code gets offered as a PR without sufficient tests. It falls upon the
> reviewer to “mentally execute” the code to trace paths that are not covered
> by tests. And, it creates a dis-incentive to make code modular, simple and
> testable. That is, because we don’t encourage unit tests, code is not
> written in a losely-coupled, modular way that would enable testing (and, I
> would argue, enable understanding.)
>
> For example, Boaz and I have wasted months working around the current
> memory model in Drill. Yes, check style improvements may have saved us
> 10-15 minutes over the life of the projects. But, good code design, and
> good unit tests, would have saved weeks.
>
> Of course, we realize that in the run-up to Drill 1.0, the goal had to be
> to get the project complete enough that the community could benefit; the
> the project succeeded in that goal. That effort has enabled us to face a
> new set of problems:
>
> * New developers who must learn and modify the code.
> * Users who want to put Drill into production.
> * The need to add new features without breaking existing code.
> * The need to maintain the existing now-legacy code.
>
> These new goals, and more, suggest a different approach to code quality
> and structure. IMO, the original rough-and-ready code structure has morphed
> from a net benefit to a net cost.
>
> My suggestions:
>
> * Make check style issues into warnings except in pre-commit builds.
> * Add check style rules as desired by the community.
> * Focus 90% of our effort on the problems that cause 90% of our wasted
> effort - the structural issues.
>
> As a committer, I would encourage the PMC to set high code design
> standards that make Drill development a joy, rather than the difficult slog
> it is today.
>
> Note also that Parth recently added “find bugs” to the Maven build. As
> Vlad noted, this did result in a very large number of warnings, many of
> which were benign. Anyone have experience how to handle this kind of issue
> effectively?
>
> Thanks,
>
> - Paul
>
>
> > On Sep 10, 2017, at 6:01 PM, Vlad Rozov <vro...@apache.org> wrote:
> >
> > +1 to add checkstyle for the Apache license header. It will be good to
> enforce consistency and avoid unnecessary warnings. I would also recommend
> to add .idea/copyright to the drill git repository to simplify adding
> license header to new files. +1 to enforce indents as well (I hope usage of
> tabs is already prohibited).
> >
> > Paul, all I was saying is that neither javac nor IntelliJ warns on the
> AutoCloseable usage outside of the try-with-resources. IntelliJ has similar
> to Eclipse code inspection, but by default it is turned off. Java compiler
> does not warn and AFAIK does not recognize @SupressWarning("resource")
> annotation. Instead of relying on IDE, I would recommend to use findbugs or
> similar static analysis tools where rules and exceptions to rules can be
> more easily formalized. My concern with using IDE to identify resource
> leaks or other bugs that may be found by a static code analysis is that
> usually there is no single IDE in use and some part of a community prefers
> IntelliJ and another part prefers Eclipse (in some communities contributors
> also use Netbeans, emacs and vi). Maintaining two sets of rules for each
> IDE is not worth the effort when the same may be achieved by using more
> advanced static analysis.
> >
> > Unfortunately using static analysis or at minimum implementing it
> initially on a large project has it's own cons. A large number of
> warnings/issues reported by such tools usually leads to denial of a tool
> usage as nobody is willing to analyze false positive and fix real issues.
> At the same time, I strongly believe that a community that implements and
> enforces static analysis will benefit in a longer run.
> >
> > The same applies to the code style enforcement (import order, placement
> of "{" and "}", indentation, white space requirements). Many project start
> without enforcing code style and as number of contributors grow realize
> that enforcing unique code style helps to avoid discussions during code
> modification and PR review and improves overall code quality and
> readability, but at that point it's hard to introduce new rules into code
> style enforcement as it would lead to a large number of violations. Some
> communities take the bullet and reformat the code to adhere to a specific
> code style.
> >
> > I agree that it is more beneficial to adhere to the best programming
> practices (small unit testable functions/classes with documentation and
> annotations) over code style. At the same time, following the best
> programming practices and enforcing unique code style do not contradict
> each other and implementing one will help implementing the other or both
> can be implemented in parallel. Code style enforcement is usually less
> costly to implement as it requires less involvement from a developer and a
> major part of the work can be accomplished by a tool.
> >
> > Thank you,
> >
> > Vlad
> >
> > On 9/9/17 06:21, Arina Yelchiyeva wrote:
> >> Also I might want to add check style for Apache header (which should be
> in
> >> a form of comment, not Javadoc), agreed code style (like indents etc)
> and
> >> enforced java doc for methods. At least the last two are enforced in
> >> Calcite.
> >> I used to point to all that stuff during code reviews but if all that
> would
> >> be enforced, it would be much easier ...
> >>
> >> Kind regards
> >> Arina
> >>
> >> On Sat, Sep 9, 2017 at 6:46 AM, Paul Rogers <prog...@mapr.com> wrote:
> >>
> >>> Hi Vlad,
> >>>
> >>> Java has a wide variety of warnings available; each project decides
> which
> >>> to ignore, which are warnings and which are errors. It may be that
> Eclipse,
> >>> by default, has resource warnings turned on. The quick & dirty
> solution is
> >>> simply to turn off warnings for AutoCloseables and missing @Overrides.
> This
> >>> is, as they say, “crude but effective."
> >>>
> >>> It seems that the Drill community stand on imports is not to change
> them.
> >>> Eclipse has an “organize imports” feature. I have to be careful when
> >>> removing unused imports not to invoke this feature as it changes import
> >>> order and often cause reviews to complain about unnecessary code
> changes.
> >>>
> >>> Would be good if we could 1) agree on a standard and 2) make sure that
> >>> both Eclipse and IntelliJ can automatically organize imports to follow
> the
> >>> standard. But, I personally don’t worry about imports because Eclipse
> takes
> >>> care of it for me.
> >>>
> >>> For me, the bigger concern is about code style. Operators are
> implemented
> >>> as huge, complex, deeply nested methods with many local variables
> (such as
> >>> flags) set one place and used elsewhere — all with no comments. Would
> seem
> >>> like a good idea to adopt best practices and require human-digestible
> >>> method sizes with good Javadoc comments. To my mind, that will
> contribute
> >>> more to the project than import order.
> >>>
> >>> Oh, and the other item that needs addressing is a requirement to create
> >>> true unit tests (not just system tests coded with JUnit.) Good unit
> test
> >>> will increase our code quality immensely, and will simplify the task
> for
> >>> code reviews. So, I’d want to push that ahead before worrying about
> imports.
> >>>
> >>> Just my two cents…
> >>>
> >>> Thanks,
> >>>
> >>> - Paul
> >>>
> >>>> On Sep 8, 2017, at 6:58 PM, Vlad Rozov <vro...@apache.org> wrote:
> >>>>
> >>>> Paul, is AutoCloseable warning specific to Eclipse? I don't remember
> >>> seeing the same warning in IntelliJ or during compilation.
> >>>> I know that some communities are significantly more strict regarding
> >>> code style and enforce not only unused imports, but also order of
> imports
> >>> and placement of static imports. What is the Drill community stand on
> those
> >>> items?
> >>>> Thank you,
> >>>>
> >>>> Vlad
> >>>>
> >>>> On 9/8/17 18:04, Paul Rogers wrote:
> >>>>> I clean up the imports as I find them, but it would be nice to do
> them
> >>> all at once to avoid the constant drip-drip-drop of warnings.
> >>>>> The key problem is the generated code: the templates can’t really
> tell
> >>> which imports are used where. So, we’d need to exclude generated code
> >>> directories from the check style rules.
> >>>>> Drill also has thousands of omitted “@Override” annotations and heavy
> >>> abuse of AutoCloseable (which triggers warnings when used outside of
> >>> try-with-resources).
> >>>>> At present, Eclipse complains about 17,883 warnings in Drill code.
> >>>>>
> >>>>> - Paul
> >>>>>
> >>>>>> On Sep 8, 2017, at 4:43 PM, Timothy Farkas <tfar...@mapr.com>
> wrote:
> >>>>>>
> >>>>>> Hi All,
> >>>>>>
> >>>>>> I've noticed that a lot of files have unused imports, and I
> frequently
> >>> accidentally leave unused imports behind when I do refactoring. So I'd
> like
> >>> to enable checkstyle to check for unused imports.
> >>>>>> Thanks,
> >>>>>> Tim
> >>>
> >
>
>
