Re: Reducing the number of warnings in the codebase
On 3/16/2014 2:48 PM, Uwe Schindler wrote: I would prefer: Before we fix warnings by 3rd party tools like Eclipse, we should first fix only the warnings emitted by Javac. The others are just unimportant to me and I don't want to fix those which are just wrong for our code style. Additional followup: With help from Steve Rowe via IRC, I got -Werror working with the ant build. Although some of the warnings I found were straightforward to eliminate, I quickly got myself into trouble when trying to fix the raw usage of Map that is all over ValueSource and its descendants. It seems that these classes cannot be restricted to specific K or V types, at least not as they are written now. Do we just suppress them? I'd like to treat it as a bug, but I'm guessing that the vague Map usage was completely intentional. On switch case fall-through: Some of them can be fixed with break statements, but a number of them are intentional. Is there a way to get the compiler to suppress the warning? I couldn't see one. Various IDEs have comment annotations that will cause the warnings to be ignored within the IDE, but there are no standards. On the IDE side, specifically for eclipse: Ignoring 'usage of a raw type' problems removes 3006 warnings -- nearly half of them. I think a large percentage of these should be fixed, and those that truly cannot be changed should be ignored with SuppressWarnings. I don't think we should actually disable this warning, for two reasons: 1) This also appears to be flagged as a warning by the compiler. 2) Properly using parameters can reveal hidden problems as errors at compile time. Long-term, I would actually go further with this -- use parameters on more of our own classes. Ignoring 'missing serialVersionUID' removes 198 warnings. We should do this. Checking the 'ignore unavoidable generic type problems' box removes 426 warnings. What exactly this might mean is not clear to me, but we should probably do this. Below are the results of some experimentation with Errors/Warnings in eclipse. I didn't try all of the options I found. Disabling deprecation warning: removes 853 warnings 'Comparing identical values' to error: adds 1 error 'Assignment has no effect' to error: adds 5 errors 'Possible accidental boolean assignment' to error: adds 4 errors 'redundant type arguments' (needs diamond) to error: adds 21 errors 'switch missing default case' to error: adds 267 errors 'switch case fallthrough' to error: adds 79 errors 'incomplete switch cases on enum' to error (but not if default present): adds 24 errors 'dead code' to error: adds 58 errors Thanks, Shawn - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
RE: Reducing the number of warnings in the codebase
On switch case fall-through: Some of them can be fixed with break statements, but a number of them are intentional. Is there a way to get the compiler to suppress the warning? I couldn't see one. Various IDEs have comment annotations that will cause the warnings to be ignored within the IDE, but there are no standards. @SuppressWarnings(fallthrough) is the official one supported by javac and eclipse. Uwe - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
Re: Reducing the number of warnings in the codebase
Hi; As being adviser of using Sonar I want to add some more points. First of all such kind of tools shows some metrics other than code warnings and we should show that metrics even we don't use them. In example *sometimes *code complexity is a good measure to review your code. I should mention that code warnings are not listed directly at Sonar. They are separated into categories. Major is an important category to take care on. You can ignore the minor warnings if you want. When I use Sonar and check the codes of my team sometimes I realize that there are false positive warnings. These rules can easily be dropped from Sonar. My idea is that: whether we care Sonar outputs or not we should integrate our project to Sonar instance available at Apache. I've opened a Jira issue for it: https://issues.apache.org/jira/browse/SOLR-5869 and *I'm volunteer to work for it.* All in all I think that these tools (as like PMD or etc.) sometimes really helpful. Try to search for the reasons of every fired bugs and at the end you can see that you've finished Effective Java because of references to items. I know that there are false positives but such things can be discard easily. Other point is that: these tools produces nice graphics that you can see the direction of your project even you don't use its bug warnings, code coverage metrics or something like that. I've created issues about bugs (I've checked all the major ones) and *I've applied patch for them* previously. Some of them are: SOLR-5836, SOLR-5838, SOLR-5839, SOLR-5840, SOLR-5841, LUCENE-5506, LUCENE-5508, LUCENE-5509 Thanks; Furkan KAMACI 2014-03-16 23:34 GMT+02:00 Benson Margulies bimargul...@gmail.com: I think we avoid bikeshed by making incremental changes. If you offer a commit to turn off serial version UID whining, I'll +1 it. And then we iterate, in small doses, agreeing to either spike the warning or change the code. In passing, I will warn you that the IDEs can be very stubborn; in some cases, there is no way to avoid some amount of whining. Eclipse used to insist on warning on every @SuppressWarnings that it didn't understand. It might still. On Sun, Mar 16, 2014 at 5:29 PM, Shawn Heisey s...@elyograg.org wrote: A starting comment: We could bikeshed for *years*. General thought: The more I think about it, the more I like the notion of confining most of the cleanup to trunk. Actual bug fixes and changes that are relatively non-invasive should be backported. On 3/16/2014 2:48 PM, Uwe Schindler wrote: Just because some tool expresses distaste, doesn't imply that everyone here agrees that it's a problem we should fix. Yes that is my biggest problem. Lots of warnings by Eclipse are just bullshit because of the code style in Lucene and for example the way we do things - e.g., it complains about missing close() all the time, just because we use IOUtils.closeWhileHandlingExceptions() for that. My original thought on this was that we should use a combination of SuppressWarnings and actual code changes to eliminate most of the warnings that show up in the well-supported IDEs when they are configured with *default* settings. Uwe brings up a really good point that there are a number of completely useless warnings, but I think there's still value in looking through EVERY default IDE warning and evaluating each one on a case-by-case basis to decide whether that specific warning should be fixed or ignored. It could be a sort of background task with an open Jira for tracking commits. It could also be something that we decide isn't worth the effort. In my experience, the default Sonar rulesets contain many things that people here are prone to disagree with. Start with serialVersionUID: do we care? Why would we care? In what cases to we really believe that a sane person would be using Java serialization with a Lucene/Solr class? We officially don't support serialization, so all warnings are useless. It's just Eclipse that complains for no reason. Project-specific IDE settings for errors/warnings (set by the ant build target) will go a long way towards making the whole situation better. For the current stable branch, we should include settings for anything that we want to ignore on trunk, but only a subset of those problems that get elevated to error status. Sonar can also be a bit cranky; it arranges for various tools to run via mechanisms that sometimes conflict with the ways you might run them yourself. So I'd suggest a process like: 1. Someone proposes a set of (e.g.) checkstyle rules to live by. 2. That ruleset is refined by experiment. 3. We make violations fail the build. Then lather, rinse, repeat for other tools. Yes I agree. I am strongly against PMD or CheckStyle without our own rules. Forbiddeen-apis was invented because of the brokenness of PMD and CheckStyle to detect default Locale/Charset/Timezone violations (and
Re: Reducing the number of warnings in the codebase
Hi; I've run FindBugs for Lucene/Solr project. If you use Intellij IDEA you can group the warnings according to their importance. I've opened issues and attached patches for top level warnings/errors (and some others) that FindBugs found. On the other hand I have another suggestion for Lucene/Solr project. When I develop or lead projects I use Sonar. It's so good and it runs really nice open source projects to analyze your code. FindBugs, PMD, Jacoco are just some of them. It also calculates the method complexities, LoC and etc. You can see a live example from here: https://sonar.springsource.org/dashboard/index/4824 I can be volunteer to integrate Sonar into Lucene/Solr project. Thanks; Furkan KAMACI 2014-03-16 11:01 GMT+02:00 Shawn Heisey s...@elyograg.org: With the default settings in Eclipse, the Lucene/Solr codebase shows over 6000 warnings. This is the case for both branch_4x and trunk. I'm no expert, but this does seem a little excessive. If I were to take on the task of reducing this number, what advice can the group give me? Is there someone in particular that I should consider a resource for inevitable dumb questions? I haven't done an exhaustive survey, but I would imagine that most of them can be eliminated fairly easily. I'm fully aware that we may not be able to eliminate them all. One problem with fixing warnings is that the resulting patch(es) would be just as invasive as the recent work to move branch_4x to Java 7. This would complicate any ongoing work, especially large-scale work that is happening onchange-specific branches. A similar topic that may require a separate discussion: FindBugs. Thanks, Shawn - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
Re: Reducing the number of warnings in the codebase
Hi Shawn, +1 for the idea, we should take full advantage of Eclipse, IntelliJ etc. Here are some relevant tickets created by Furkan. https://issues.apache.org/jira/browse/LUCENE-5506 https://issues.apache.org/jira/browse/SOLR-5838 https://issues.apache.org/jira/browse/SOLR-5839 I believe https://issues.apache.org/jira/browse/SOLR-5685 could expressed as an automatic rule or something. There is already similar thing to detect usage of String.uppercase/lowercase without locale. And StringBuffer versus StringBuilder. Ahmet On Sunday, March 16, 2014 12:09 PM, Furkan KAMACI furkankam...@gmail.com wrote: Hi; I've run FindBugs for Lucene/Solr project. If you use Intellij IDEA you can group the warnings according to their importance. I've opened issues and attached patches for top level warnings/errors (and some others) that FindBugs found. On the other hand I have another suggestion for Lucene/Solr project. When I develop or lead projects I use Sonar. It's so good and it runs really nice open source projects to analyze your code. FindBugs, PMD, Jacoco are just some of them. It also calculates the method complexities, LoC and etc. You can see a live example from here: https://sonar.springsource.org/dashboard/index/4824 I can be volunteer to integrate Sonar into Lucene/Solr project. Thanks; Furkan KAMACI 2014-03-16 11:01 GMT+02:00 Shawn Heisey s...@elyograg.org: With the default settings in Eclipse, the Lucene/Solr codebase shows over 6000 warnings. This is the case for both branch_4x and trunk. I'm no expert, but this does seem a little excessive. If I were to take on the task of reducing this number, what advice can the group give me? Is there someone in particular that I should consider a resource for inevitable dumb questions? I haven't done an exhaustive survey, but I would imagine that most of them can be eliminated fairly easily. I'm fully aware that we may not be able to eliminate them all. One problem with fixing warnings is that the resulting patch(es) would be just as invasive as the recent work to move branch_4x to Java 7. This would complicate any ongoing work, especially large-scale work that is happening onchange-specific branches. A similar topic that may require a separate discussion: FindBugs. Thanks, Shawn - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
Re: Reducing the number of warnings in the codebase
On Sun, Mar 16, 2014 at 6:09 AM, Furkan KAMACI furkankam...@gmail.com wrote: I've run FindBugs for Lucene/Solr project. If you use Intellij IDEA you can group the warnings according to their importance. I've opened issues and attached patches for top level warnings/errors (and some others) that FindBugs found. On the other hand I have another suggestion for Lucene/Solr project. When I develop or lead projects I use Sonar. It's so good and it runs really nice open source projects to analyze your code. FindBugs, PMD, Jacoco are just some of them. It also calculates the method complexities, LoC and etc. You can see a live example from here: https://sonar.springsource.org/dashboard/index/4824 +1, Sonar looks really nice! I can be volunteer to integrate Sonar into Lucene/Solr project. Thank you Furkan. Mike McCandless http://blog.mikemccandless.com - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
Re: Reducing the number of warnings in the codebase
hİ; Thanks Michael. I will open a Jira issue for it. Thanks; Furkan KAMACI 2014-03-16 13:35 GMT+02:00 Michael McCandless luc...@mikemccandless.com: On Sun, Mar 16, 2014 at 6:09 AM, Furkan KAMACI furkankam...@gmail.com wrote: I've run FindBugs for Lucene/Solr project. If you use Intellij IDEA you can group the warnings according to their importance. I've opened issues and attached patches for top level warnings/errors (and some others) that FindBugs found. On the other hand I have another suggestion for Lucene/Solr project. When I develop or lead projects I use Sonar. It's so good and it runs really nice open source projects to analyze your code. FindBugs, PMD, Jacoco are just some of them. It also calculates the method complexities, LoC and etc. You can see a live example from here: https://sonar.springsource.org/dashboard/index/4824 +1, Sonar looks really nice! I can be volunteer to integrate Sonar into Lucene/Solr project. Thank you Furkan. Mike McCandless http://blog.mikemccandless.com - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
Re: Reducing the number of warnings in the codebase
Hi; Here is Apache projects that is analyzed via Sonar: https://analysis.apache.org/all_projects?qualifier=TRK Thanks; Furkan KAMACI 2014-03-16 15:37 GMT+02:00 Furkan KAMACI furkankam...@gmail.com: hİ; Thanks Michael. I will open a Jira issue for it. Thanks; Furkan KAMACI 2014-03-16 13:35 GMT+02:00 Michael McCandless luc...@mikemccandless.com: On Sun, Mar 16, 2014 at 6:09 AM, Furkan KAMACI furkankam...@gmail.com wrote: I've run FindBugs for Lucene/Solr project. If you use Intellij IDEA you can group the warnings according to their importance. I've opened issues and attached patches for top level warnings/errors (and some others) that FindBugs found. On the other hand I have another suggestion for Lucene/Solr project. When I develop or lead projects I use Sonar. It's so good and it runs really nice open source projects to analyze your code. FindBugs, PMD, Jacoco are just some of them. It also calculates the method complexities, LoC and etc. You can see a live example from here: https://sonar.springsource.org/dashboard/index/4824 +1, Sonar looks really nice! I can be volunteer to integrate Sonar into Lucene/Solr project. Thank you Furkan. Mike McCandless http://blog.mikemccandless.com - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
Re: Reducing the number of warnings in the codebase
On 3/16/2014 5:35 AM, Michael McCandless wrote: On Sun, Mar 16, 2014 at 6:09 AM, Furkan KAMACI furkankam...@gmail.com wrote: I've run FindBugs for Lucene/Solr project. If you use Intellij IDEA you can group the warnings according to their importance. I've opened issues and attached patches for top level warnings/errors (and some others) that FindBugs found. On the other hand I have another suggestion for Lucene/Solr project. When I develop or lead projects I use Sonar. It's so good and it runs really nice open source projects to analyze your code. FindBugs, PMD, Jacoco are just some of them. It also calculates the method complexities, LoC and etc. You can see a live example from here: https://sonar.springsource.org/dashboard/index/4824 +1, Sonar looks really nice! I can be volunteer to integrate Sonar into Lucene/Solr project. Thank you Furkan. I haven't yet looked, but the *idea* of Sonar sounds quite awesome. +1 for me on a thank you, Furkan. The ides of march spam from Jira on version 4.7 had one benefit relating to this discussion: It put LUCENE-5130 on my radar. Would it be too much administrative @#!* to create an umbrella issue? I'd suggest LUCENE-5130 for this purpose, except that I'm not 100% positive that failing the build is the right answer. I fully understand the motivation ... it would certainly force us to face the issue! A bunch of smaller issues could be created to tackle subsections of the code, or perhaps to tackle a particular type of warning. This really doesn't change how invasive the patches would be, but if they come in smaller chunks, it might be easier to work around them. When it comes to warnings about things like missing serialVersionUID, should we generate a random number for each class, or use a default value? Thanks, Shawn - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
Re: Reducing the number of warnings in the codebase
On 3/16/2014 12:26 PM, Shawn Heisey wrote: Would it be too much administrative @#!* to create an umbrella issue? I'd suggest LUCENE-5130 for this purpose, except that I'm not 100% positive that failing the build is the right answer. I fully understand the motivation ... it would certainly force us to face the issue! A bunch of smaller issues could be created to tackle subsections of the code, or perhaps to tackle a particular type of warning. This really doesn't change how invasive the patches would be, but if they come in smaller chunks, it might be easier to work around them. When it comes to warnings about things like missing serialVersionUID, should we generate a random number for each class, or use a default value? A further idea: We could limit this cleanup to trunk. I foresee three main effects, none of which seems like a bad thing to me: * We don't risk breaking the stable branch. * The cleanup might reveal actual bugs or clearly broken code. * Backporting gets harder, pushing us closer to the 5.0 release. Thanks, Shawn - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
Re: Reducing the number of warnings in the codebase
Just because some tool expresses distaste, doesn't imply that everyone here agrees that it's a problem we should fix. In my experience, the default Sonar rulesets contain many things that people here are prone to disagree with. Start with serialVersionUID: do we care? Why would we care? In what cases to we really believe that a sane person would be using Java serialization with a Lucene/Solr class? Sonar can also be a bit cranky; it arranges for various tools to run via mechanisms that sometimes conflict with the ways you might run them yourself. So I'd suggest a process like: 1. Someone proposes a set of (e.g.) checkstyle rules to live by. 2. That ruleset is refined by experiment. 3. We make violations fail the build. Then lather, rinse, repeat for other tools. Once we have rulesets we agree are worth enforcing, we can look to Sonar for a pretty way to visualize their results if we like. - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
Re: Reducing the number of warnings in the codebase
Hi, Here are some rules : Following String methods where left hand side is empty. String.replace() String.toUpperCase() String.toLowerCase() String.replaceFirst() String.trim() In test cases (subblasses of SolrTestCaseJ4) methods without assertU(). see : SOLR-5685 adoc() optimize() commit() String.toUpperCase() and String.toLowerCase() without Locale. see : SOLR-2281 and LUCENE-2466 Can ant precommit/forbidden-apis be used to detect above? Ahmet On Sunday, March 16, 2014 9:53 PM, Benson Margulies bimargul...@gmail.com wrote: Just because some tool expresses distaste, doesn't imply that everyone here agrees that it's a problem we should fix. In my experience, the default Sonar rulesets contain many things that people here are prone to disagree with. Start with serialVersionUID: do we care? Why would we care? In what cases to we really believe that a sane person would be using Java serialization with a Lucene/Solr class? Sonar can also be a bit cranky; it arranges for various tools to run via mechanisms that sometimes conflict with the ways you might run them yourself. So I'd suggest a process like: 1. Someone proposes a set of (e.g.) checkstyle rules to live by. 2. That ruleset is refined by experiment. 3. We make violations fail the build. Then lather, rinse, repeat for other tools. Once we have rulesets we agree are worth enforcing, we can look to Sonar for a pretty way to visualize their results if we like. - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
RE: Reducing the number of warnings in the codebase
String.toUpperCase() and String.toLowerCase() without Locale. see : SOLR- 2281 and LUCENE-2466 Those are already detected by forbidden-apis. Can ant precommit/forbidden-apis be used to detect above? Ahmet On Sunday, March 16, 2014 9:53 PM, Benson Margulies bimargul...@gmail.com wrote: Just because some tool expresses distaste, doesn't imply that everyone here agrees that it's a problem we should fix. In my experience, the default Sonar rulesets contain many things that people here are prone to disagree with. Start with serialVersionUID: do we care? Why would we care? In what cases to we really believe that a sane person would be using Java serialization with a Lucene/Solr class? Sonar can also be a bit cranky; it arranges for various tools to run via mechanisms that sometimes conflict with the ways you might run them yourself. So I'd suggest a process like: 1. Someone proposes a set of (e.g.) checkstyle rules to live by. 2. That ruleset is refined by experiment. 3. We make violations fail the build. Then lather, rinse, repeat for other tools. Once we have rulesets we agree are worth enforcing, we can look to Sonar for a pretty way to visualize their results if we like. - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
RE: Reducing the number of warnings in the codebase
Hi, Just because some tool expresses distaste, doesn't imply that everyone here agrees that it's a problem we should fix. Yes that is my biggest problem. Lots of warnings by Eclipse are just bullshit because of the code style in Lucene and for example the way we do things - e.g., it complains about missing close() all the time, just because we use IOUtils.closeWhileHandlingExceptions() for that. In my experience, the default Sonar rulesets contain many things that people here are prone to disagree with. Start with serialVersionUID: do we care? Why would we care? In what cases to we really believe that a sane person would be using Java serialization with a Lucene/Solr class? We officially don't support serialization, so all warnings are useless. It's just Eclipse that complains for no reason. Sonar can also be a bit cranky; it arranges for various tools to run via mechanisms that sometimes conflict with the ways you might run them yourself. So I'd suggest a process like: 1. Someone proposes a set of (e.g.) checkstyle rules to live by. 2. That ruleset is refined by experiment. 3. We make violations fail the build. Then lather, rinse, repeat for other tools. Yes I agree. I am strongly against PMD or CheckStyle without our own rules. Forbiddeen-apis was invented because of the brokenness of PMD and CheckStyle to detect default Locale/Charset/Timezone violations (and also because those tools are slow). We should better fix out Eclipse Project generate to hide the warnings that are just wrong. I would prefer: Before we fix warnings by 3rd party tools like Eclipse, we should first fix only the warnings emitted by Javac. The others are just unimportant to me and I don't want to fix those which are just wrong for our code style. We already have ECJ in our build (to lint javadocs), we can make some Eclipse warnings fatal through the ecj config file in our SVN, to fail build on some warnings. I disagree with using PMD or Checkstyle, those tools are uncomplete and broken, sorry. Uwe Once we have rulesets we agree are worth enforcing, we can look to Sonar for a pretty way to visualize their results if we like. - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
Re: Reducing the number of warnings in the codebase
Hi Uwe, I looked for definitions under lucene/tools/forbiddenApis/*.txt files but I couldn't find. Where are those rule are defined? I am wondering about the syntax, can you point? Thanks, Ahmet On Sunday, March 16, 2014 10:40 PM, Uwe Schindler u...@thetaphi.de wrote: String.toUpperCase() and String.toLowerCase() without Locale. see : SOLR- 2281 and LUCENE-2466 Those are already detected by forbidden-apis. Can ant precommit/forbidden-apis be used to detect above? Ahmet On Sunday, March 16, 2014 9:53 PM, Benson Margulies bimargul...@gmail.com wrote: Just because some tool expresses distaste, doesn't imply that everyone here agrees that it's a problem we should fix. In my experience, the default Sonar rulesets contain many things that people here are prone to disagree with. Start with serialVersionUID: do we care? Why would we care? In what cases to we really believe that a sane person would be using Java serialization with a Lucene/Solr class? Sonar can also be a bit cranky; it arranges for various tools to run via mechanisms that sometimes conflict with the ways you might run them yourself. So I'd suggest a process like: 1. Someone proposes a set of (e.g.) checkstyle rules to live by. 2. That ruleset is refined by experiment. 3. We make violations fail the build. Then lather, rinse, repeat for other tools. Once we have rulesets we agree are worth enforcing, we can look to Sonar for a pretty way to visualize their results if we like. - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
Re: Reducing the number of warnings in the codebase
On 3/16/2014 1:52 PM, Benson Margulies wrote: Just because some tool expresses distaste, doesn't imply that everyone here agrees that it's a problem we should fix. I had most of a reply written before I saw Uwe's response. He brings up some good points that made me re-think important parts of what I was saying. So, I start over. :) Thanks, Shawn - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
Re: Reducing the number of warnings in the codebase
That are the default rules in the jar file. All so called unsafe methods and classes. The list is immense. http://code.google.com/p/forbidden-apis/source/browse/trunk#trunk%2Fsrc%2Fmain%2Fresources%2Fde%2Fthetaphi%2Fforbiddenapis%2Fsignatures See also homepage of tool and its jar file. Uwe On 16. März 2014 21:53:46 MEZ, Ahmet Arslan iori...@yahoo.com wrote: Hi Uwe, I looked for definitions under lucene/tools/forbiddenApis/*.txt files but I couldn't find. Where are those rule are defined? I am wondering about the syntax, can you point? Thanks, Ahmet On Sunday, March 16, 2014 10:40 PM, Uwe Schindler u...@thetaphi.de wrote: String.toUpperCase() and String.toLowerCase() without Locale. see : SOLR- 2281 and LUCENE-2466 Those are already detected by forbidden-apis. Can ant precommit/forbidden-apis be used to detect above? Ahmet On Sunday, March 16, 2014 9:53 PM, Benson Margulies bimargul...@gmail.com wrote: Just because some tool expresses distaste, doesn't imply that everyone here agrees that it's a problem we should fix. In my experience, the default Sonar rulesets contain many things that people here are prone to disagree with. Start with serialVersionUID: do we care? Why would we care? In what cases to we really believe that a sane person would be using Java serialization with a Lucene/Solr class? Sonar can also be a bit cranky; it arranges for various tools to run via mechanisms that sometimes conflict with the ways you might run them yourself. So I'd suggest a process like: 1. Someone proposes a set of (e.g.) checkstyle rules to live by. 2. That ruleset is refined by experiment. 3. We make violations fail the build. Then lather, rinse, repeat for other tools. Once we have rulesets we agree are worth enforcing, we can look to Sonar for a pretty way to visualize their results if we like. - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org -- Uwe Schindler H.-H.-Meier-Allee 63, 28213 Bremen http://www.thetaphi.de
Re: Reducing the number of warnings in the codebase
A starting comment: We could bikeshed for *years*. General thought: The more I think about it, the more I like the notion of confining most of the cleanup to trunk. Actual bug fixes and changes that are relatively non-invasive should be backported. On 3/16/2014 2:48 PM, Uwe Schindler wrote: Just because some tool expresses distaste, doesn't imply that everyone here agrees that it's a problem we should fix. Yes that is my biggest problem. Lots of warnings by Eclipse are just bullshit because of the code style in Lucene and for example the way we do things - e.g., it complains about missing close() all the time, just because we use IOUtils.closeWhileHandlingExceptions() for that. My original thought on this was that we should use a combination of SuppressWarnings and actual code changes to eliminate most of the warnings that show up in the well-supported IDEs when they are configured with *default* settings. Uwe brings up a really good point that there are a number of completely useless warnings, but I think there's still value in looking through EVERY default IDE warning and evaluating each one on a case-by-case basis to decide whether that specific warning should be fixed or ignored. It could be a sort of background task with an open Jira for tracking commits. It could also be something that we decide isn't worth the effort. In my experience, the default Sonar rulesets contain many things that people here are prone to disagree with. Start with serialVersionUID: do we care? Why would we care? In what cases to we really believe that a sane person would be using Java serialization with a Lucene/Solr class? We officially don't support serialization, so all warnings are useless. It's just Eclipse that complains for no reason. Project-specific IDE settings for errors/warnings (set by the ant build target) will go a long way towards making the whole situation better. For the current stable branch, we should include settings for anything that we want to ignore on trunk, but only a subset of those problems that get elevated to error status. Sonar can also be a bit cranky; it arranges for various tools to run via mechanisms that sometimes conflict with the ways you might run them yourself. So I'd suggest a process like: 1. Someone proposes a set of (e.g.) checkstyle rules to live by. 2. That ruleset is refined by experiment. 3. We make violations fail the build. Then lather, rinse, repeat for other tools. Yes I agree. I am strongly against PMD or CheckStyle without our own rules. Forbiddeen-apis was invented because of the brokenness of PMD and CheckStyle to detect default Locale/Charset/Timezone violations (and also because those tools are slow). We should better fix out Eclipse Project generate to hide the warnings that are just wrong. I would prefer: Before we fix warnings by 3rd party tools like Eclipse, we should first fix only the warnings emitted by Javac. The others are just unimportant to me and I don't want to fix those which are just wrong for our code style. We already have ECJ in our build (to lint javadocs), we can make some Eclipse warnings fatal through the ecj config file in our SVN, to fail build on some warnings. I disagree with using PMD or Checkstyle, those tools are uncomplete and broken, sorry. +1 all around. I want to eliminate all the noise. If we had IDE warnings measured in dozens instead of thousands, it would be a useful data point that wouldn't get ignored. Thanks, Shawn - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
Re: Reducing the number of warnings in the codebase
I think we avoid bikeshed by making incremental changes. If you offer a commit to turn off serial version UID whining, I'll +1 it. And then we iterate, in small doses, agreeing to either spike the warning or change the code. In passing, I will warn you that the IDEs can be very stubborn; in some cases, there is no way to avoid some amount of whining. Eclipse used to insist on warning on every @SuppressWarnings that it didn't understand. It might still. On Sun, Mar 16, 2014 at 5:29 PM, Shawn Heisey s...@elyograg.org wrote: A starting comment: We could bikeshed for *years*. General thought: The more I think about it, the more I like the notion of confining most of the cleanup to trunk. Actual bug fixes and changes that are relatively non-invasive should be backported. On 3/16/2014 2:48 PM, Uwe Schindler wrote: Just because some tool expresses distaste, doesn't imply that everyone here agrees that it's a problem we should fix. Yes that is my biggest problem. Lots of warnings by Eclipse are just bullshit because of the code style in Lucene and for example the way we do things - e.g., it complains about missing close() all the time, just because we use IOUtils.closeWhileHandlingExceptions() for that. My original thought on this was that we should use a combination of SuppressWarnings and actual code changes to eliminate most of the warnings that show up in the well-supported IDEs when they are configured with *default* settings. Uwe brings up a really good point that there are a number of completely useless warnings, but I think there's still value in looking through EVERY default IDE warning and evaluating each one on a case-by-case basis to decide whether that specific warning should be fixed or ignored. It could be a sort of background task with an open Jira for tracking commits. It could also be something that we decide isn't worth the effort. In my experience, the default Sonar rulesets contain many things that people here are prone to disagree with. Start with serialVersionUID: do we care? Why would we care? In what cases to we really believe that a sane person would be using Java serialization with a Lucene/Solr class? We officially don't support serialization, so all warnings are useless. It's just Eclipse that complains for no reason. Project-specific IDE settings for errors/warnings (set by the ant build target) will go a long way towards making the whole situation better. For the current stable branch, we should include settings for anything that we want to ignore on trunk, but only a subset of those problems that get elevated to error status. Sonar can also be a bit cranky; it arranges for various tools to run via mechanisms that sometimes conflict with the ways you might run them yourself. So I'd suggest a process like: 1. Someone proposes a set of (e.g.) checkstyle rules to live by. 2. That ruleset is refined by experiment. 3. We make violations fail the build. Then lather, rinse, repeat for other tools. Yes I agree. I am strongly against PMD or CheckStyle without our own rules. Forbiddeen-apis was invented because of the brokenness of PMD and CheckStyle to detect default Locale/Charset/Timezone violations (and also because those tools are slow). We should better fix out Eclipse Project generate to hide the warnings that are just wrong. I would prefer: Before we fix warnings by 3rd party tools like Eclipse, we should first fix only the warnings emitted by Javac. The others are just unimportant to me and I don't want to fix those which are just wrong for our code style. We already have ECJ in our build (to lint javadocs), we can make some Eclipse warnings fatal through the ecj config file in our SVN, to fail build on some warnings. I disagree with using PMD or Checkstyle, those tools are uncomplete and broken, sorry. +1 all around. I want to eliminate all the noise. If we had IDE warnings measured in dozens instead of thousands, it would be a useful data point that wouldn't get ignored. Thanks, Shawn - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
RE: Reducing the number of warnings in the codebase
I agree with uwe. Start with javac, use -Werror flag and fix those first. Stupid warnings like serialization are already disabled for javac in the build. On Mar 16, 2014 4:48 PM, Uwe Schindler u...@thetaphi.de wrote: Hi, Just because some tool expresses distaste, doesn't imply that everyone here agrees that it's a problem we should fix. Yes that is my biggest problem. Lots of warnings by Eclipse are just bullshit because of the code style in Lucene and for example the way we do things - e.g., it complains about missing close() all the time, just because we use IOUtils.closeWhileHandlingExceptions() for that. In my experience, the default Sonar rulesets contain many things that people here are prone to disagree with. Start with serialVersionUID: do we care? Why would we care? In what cases to we really believe that a sane person would be using Java serialization with a Lucene/Solr class? We officially don't support serialization, so all warnings are useless. It's just Eclipse that complains for no reason. Sonar can also be a bit cranky; it arranges for various tools to run via mechanisms that sometimes conflict with the ways you might run them yourself. So I'd suggest a process like: 1. Someone proposes a set of (e.g.) checkstyle rules to live by. 2. That ruleset is refined by experiment. 3. We make violations fail the build. Then lather, rinse, repeat for other tools. Yes I agree. I am strongly against PMD or CheckStyle without our own rules. Forbiddeen-apis was invented because of the brokenness of PMD and CheckStyle to detect default Locale/Charset/Timezone violations (and also because those tools are slow). We should better fix out Eclipse Project generate to hide the warnings that are just wrong. I would prefer: Before we fix warnings by 3rd party tools like Eclipse, we should first fix only the warnings emitted by Javac. The others are just unimportant to me and I don't want to fix those which are just wrong for our code style. We already have ECJ in our build (to lint javadocs), we can make some Eclipse warnings fatal through the ecj config file in our SVN, to fail build on some warnings. I disagree with using PMD or Checkstyle, those tools are uncomplete and broken, sorry. Uwe Once we have rulesets we agree are worth enforcing, we can look to Sonar for a pretty way to visualize their results if we like. - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
