drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul Rogers <prog...@mapr.com>
Subject Re: Checkstyle Unused Imports
Date Sat, 09 Sep 2017 03:46:50 GMT
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 <vrozov@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 <tfarkas@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
> 

Mime
View raw message