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
