Checkstyle Unused Imports

[Timothy Farkas <tf...@mapr.com>]

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

Re: Checkstyle Unused Imports

[Ted Dunning <te...@gmail.com>]

On Mon, Sep 11, 2017 at 9:48 PM, Julian Hyde <jh...@apache.org> wrote:

> The options Ted is thinking of are:
>
> -Dcheckstyle.skip
> -DskipTests
>

It is sooo cool to have well-informed people around!

Thanks Julian.

Re: Checkstyle Unused Imports

[Julian Hyde <jh...@apache.org>]

The options Ted is thinking of are:

-Dcheckstyle.skip
-DskipTests

Julian


> On Sep 11, 2017, at 12:46 PM, Ted Dunning <te...@gmail.com> wrote:
> 
> On Mon, Sep 11, 2017 at 6:53 PM, Paul Rogers <pr...@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 is easily defeated when you run with an option similar to the
> way that unit tests are commonly defeated during development.
> 
> I don't remember the exact option.

Re: Checkstyle Unused Imports

[Ted Dunning <te...@gmail.com>]

On Mon, Sep 11, 2017 at 6:53 PM, Paul Rogers <pr...@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 is easily defeated when you run with an option similar to the
way that unit tests are commonly defeated during development.

I don't remember the exact option.

Checkstyle error or warning (was: Checkstyle Unused Imports)

[Vlad Rozov <vr...@apache.org>]

I think that it is not necessary to or even undesirable to convert code 
style (checkstyle) errors into warnings. I am not sure how it is handled 
in Drill, in Apex we use -Dcheckstyle.skip=true to disable checkstyle 
plugin during development builds. If the community prefers, another 
option would be to unbind checkstyle from maven phase and invoke it 
explicitly during CI build. The second option will require contributors 
(and committers) to check the status of CI build and fix checkstyle 
errors if necessary. I'd recommend to do the same for apache-rat plugin. 
It is not necessary to run the plugin on every build, especially that 
local workspace may have logs and other temporary files with unapproved 
licenses. The apache-rat plugin can be explicitly invoked during CI 
build and fail the build.

I would also recommend to install checkstyle IntelliJ  plugin (I guess 
plugin for Eclipse also exists). The plugin provides ability to scan 
file/module/project for checkstyle errors and quickly fix them. It also 
integrates with IntelliJ intentions and marks violations in real-time if 
enabled.

Forget to note, both IntelliJ and Eclipse provide ability to define 
order of imports. I'll start a separate thread to discuss the community 
opinion on this subject.

Thank you,

Vlad

On 9/11/17 10:44, Aman Sinha wrote:
> 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 <pr...@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 <vr...@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 <pr...@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 <vr...@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 <tf...@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
>>

Re: Checkstyle Unused Imports

[Aman Sinha <am...@apache.org>]

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 <pr...@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 <vr...@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 <pr...@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 <vr...@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 <tf...@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
> >>>
> >
>
>

Re: Checkstyle Unused Imports

[Paul Rogers <pr...@mapr.com>]

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 <vr...@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 <pr...@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 <vr...@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 <tf...@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
>>> 
> 

Re: Checkstyle Unused Imports

[Vlad Rozov <vr...@apache.org>]

+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 <pr...@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 <vr...@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 <tf...@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
>>

Re: Checkstyle Unused Imports

[Arina Yelchiyeva <ar...@gmail.com>]

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 <pr...@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 <vr...@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 <tf...@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
> >
>
>

Re: Checkstyle Unused Imports

[Paul Rogers <pr...@mapr.com>]

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 <vr...@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 <tf...@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
> 

Re: Checkstyle Unused Imports

[Vlad Rozov <vr...@apache.org>]

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 <tf...@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

Re: Checkstyle Unused Imports

[Paul Rogers <pr...@mapr.com>]

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 <tf...@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