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 <[email protected]> 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, - PaulOn Sep 8, 2017, at 6:58 PM, Vlad Rozov <[email protected]> wrote: Paul, is AutoCloseable warning specific to Eclipse? I don't rememberseeing the same warning in IntelliJ or during compilation.I know that some communities are significantly more strict regardingcode 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 themall at once to avoid the constant drip-drip-drop of warnings.The key problem is the generated code: the templates can’t really tellwhich 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 heavyabuse of AutoCloseable (which triggers warnings when used outside of try-with-resources).At present, Eclipse complains about 17,883 warnings in Drill code. - PaulOn Sep 8, 2017, at 4:43 PM, Timothy Farkas <[email protected]> wrote: Hi All, I've noticed that a lot of files have unused imports, and I frequentlyaccidentally leave unused imports behind when I do refactoring. So I'd like to enable checkstyle to check for unused imports.Thanks, Tim
