So far I've reviewed only Experimental.java... https://codereview.appspot.com/7372049/diff/1/google-http-client/src/main/java/com/google/api/client/util/Experimental.java File google-http-client/src/main/java/com/google/api/client/util/Experimental.java (right): https://codereview.appspot.com/7372049/diff/1/google-http-client/src/main/java/com/google/api/client/util/Experimental.java#newcode22 google-http-client/src/main/java/com/google/api/client/util/Experimental.java:22: * subject to ...
11 years, 2 months ago
(2013-02-25 14:58:55 UTC)
#1
So far I've reviewed only Experimental.java...
https://codereview.appspot.com/7372049/diff/1/google-http-client/src/main/jav...
File
google-http-client/src/main/java/com/google/api/client/util/Experimental.java
(right):
https://codereview.appspot.com/7372049/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:22:
* subject to incompatible changes or removal.
It's potentially even worse than that, because it may also mean that the server
side feature it depends on is subject to incompatible changes or removal at any
time. Developers need to read the JavaDoc of the API for details for a better
understanding of the risk.
https://codereview.appspot.com/7372049/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:22:
* subject to incompatible changes or removal.
add "in a future release"
https://codereview.appspot.com/7372049/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:24:
* Note that the presence of this annotation implies nothing about the quality
or performance of
surround with <p> and </p>
https://codereview.appspot.com/7372049/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:24:
* Note that the presence of this annotation implies nothing about the quality
or performance of
Please remove this sentence. The presence of the "Experimental" annotation may
imply something about the quality or performance of the API in question. That
may well have been the reason we marked it as Experimental.
https://codereview.appspot.com/7372049/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:24:
* Note that the presence of this annotation implies nothing about the quality
or performance of
Add a sentence to explain that a public API with this annotation "is exempt from
any compatibility guarantees" like Guava has on @Beta. That said, if we decide
to remove an experimental API, we have a general policy that it should be
deprecated in the first minor release, and removed in the second release. That
gives client applications a smoother upgrade path.
https://codereview.appspot.com/7372049/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:25:
* the API in question, only the fact that it is not "API-frozen".
Add a sentence that it is generally inadvisable for other non-experimental
libraries to use experimental API from this library. The problem is that other
libraries don't have control over the version of this library being used in
client applications, and if the wrong version of this library is used, it has
the potential to break client applications.
https://codereview.appspot.com/7372049/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:25:
* the API in question, only the fact that it is not "API-frozen".
should we mention the FindBugs plugin in the JavaDoc here?
https://codereview.appspot.com/7372049/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:34:
public @interface Experimental {
please add @Documented annotation. See:
http://docs.oracle.com/javase/7/docs/api/java/lang/annotation/Documented.html
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/.classpath File google-http-client-findbugs/.classpath (right): https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/.classpath#newcode5 google-http-client-findbugs/.classpath:5: <classpathentry kind="src" path="src/test/java"/> src/test/java - was removed. On 2013/02/26 ...
11 years, 1 month ago
(2013-02-27 15:32:38 UTC)
#3
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/.cl...
File google-http-client-findbugs/.classpath (right):
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/.cl...
google-http-client-findbugs/.classpath:5: <classpathentry kind="src"
path="src/test/java"/>
src/test/java - was removed.
On 2013/02/26 03:12:07, yanivi wrote:
> Description Resource Path Location Type
> Project 'google-http-client-findbugs' is missing required source folder:
> 'src/test/java' google-http-client-findbugs Build path Build Path Problem
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/goo...
File google-http-client-findbugs/google-http-client-findbugs-test/.classpath
(right):
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/goo...
google-http-client-findbugs/google-http-client-findbugs-test/.classpath:4:
<classpathentry kind="src" path="src/main/resources"/>
On 2013/02/26 03:12:07, yanivi wrote:
> Description Resource Path Location Type
> Project 'google-http-client-findbugs-test' is missing required source folder:
> 'src/main/resources' google-http-client-findbugs-test Build path Build Path
> Problem
> Project 'google-http-client-findbugs-test' is missing required source folder:
> 'src/test/java' google-http-client-findbugs-test Build path Build Path
Problem
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/goo...
File
google-http-client-findbugs/google-http-client-findbugs-test/.settings/org.eclipse.jdt.core.prefs
(right):
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/goo...
google-http-client-findbugs/google-http-client-findbugs-test/.settings/org.eclipse.jdt.core.prefs:1:
eclipse.preferences.version=1
On 2013/02/26 03:12:07, yanivi wrote:
> please copy from google-http-client:
> .settings/org.eclipse.jdt.core.prefs
> .settings/org.eclipse.jdt.ui.prefs
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/goo...
File google-http-client-findbugs/google-http-client-findbugs-test/pom.xml
(right):
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/goo...
google-http-client-findbugs/google-http-client-findbugs-test/pom.xml:17:
<artifactId>maven-compiler-plugin</artifactId>
I don't see it,
maybe it was because the library (google-http-clientfindbugs) haven't been build
in your end.
Please try again
On 2013/02/26 03:12:07, yanivi wrote:
> [WARNING] Some problems were encountered while building the effective model
for
>
com.google.http-client:google-http-client-findbugs-test:jar:1.14.0-beta-SNAPSHOT
> [WARNING] 'build.plugins.plugin.(groupId:artifactId)' must be unique but found
> duplicate declaration of plugin org.apache.maven.plugins:maven-compiler-plugin
@
> line 36, column 15
> [WARNING] 'parent.relativePath' points at
> com.google.http-client:google-http-client-findbugs instead of
com.google:google,
> please verify your project structure @ line 4, column 11
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/goo...
File
google-http-client-findbugs/google-http-client-findbugs-test/src/main/java/com/google/api/client/findbugs/testing/ClassWithExperimentalField.java
(right):
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/goo...
google-http-client-findbugs/google-http-client-findbugs-test/src/main/java/com/google/api/client/findbugs/testing/ClassWithExperimentalField.java:15:
package com.google.api.client.findbugs.testing;
On 2013/02/26 03:12:07, yanivi wrote:
> don't use "testing". Perhaps "test"?
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/goo...
google-http-client-findbugs/google-http-client-findbugs-test/src/main/java/com/google/api/client/findbugs/testing/ClassWithExperimentalField.java:34:
return "Field: " + field + ", Experimental Field: " + experimentalField;
I managed to load a plugin to FindBugs GUI by:
Edit -> Preferences -> Plugin -> Install new plugin... -> [load the jar]
On 2013/02/26 03:12:07, yanivi wrote:
> I don't know if it is fixable, but when I use FindBugs GUI (mvn findbugs:gui)
I
> get this description for the problem:
>
> Unknown bug pattern
> A warning was recorded, but findbugs can't find the description of this bug
> pattern and so can't describe it. This should occur only in cases of a bug in
> FindBugs or its configuration, or perhaps if an analysis was generated using a
> plugin, but that plugin is not currently loaded. .
>
> Bug kind and pattern: TEST - UNKNOWN
>
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/goo...
File
google-http-client-findbugs/google-http-client-findbugs-test/src/main/java/com/google/api/client/findbugs/testing/package-info.java
(right):
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/goo...
google-http-client-findbugs/google-http-client-findbugs-test/src/main/java/com/google/api/client/findbugs/testing/package-info.java:24:
package com.google.api.client.findbugs.testing;
Done.
I just saw that other test packages don't use a package-info, so I removed this
one.
On 2013/02/26 03:12:07, yanivi wrote:
> do we really need a package-info.java file? doesn't seem needed at all in a
> test package.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/pom...
File google-http-client-findbugs/pom.xml (right):
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/pom...
google-http-client-findbugs/pom.xml:15:
<artifactId>maven-javadoc-plugin</artifactId>
Done.
On 2013/02/26 03:12:07, yanivi wrote:
> remove javadoc-plugin
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/pom...
google-http-client-findbugs/pom.xml:46: <version>2.0.0</version>
On 2013/02/26 03:12:07, yanivi wrote:
> versions of dependencies is managed in ../pom.xml in the dependencyManagement
> section
That's problematic, because in google-http-client-findbugs-test I'm not using
the parent pom.xml. so I leave it like that.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
File
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java
(right):
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:30:
* Findbugs plugin detector which detects usage of @Experimental in your code.
On 2013/02/26 03:12:07, yanivi wrote:
> {@link Experimental} annotation
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:32:
* @since 1.15
On 2013/02/26 03:12:07, yanivi wrote:
> remove @since
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:34:
*
On 2013/02/26 03:12:07, yanivi wrote:
> remove line
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:38:
/**
On 2013/02/26 03:12:07, yanivi wrote:
> [optional] For one-line JavaDoc I prefer the style:
> /** Experimental annotation "signature". */
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:64:
* Constructs a new instance with Findbugs' <code>BugReporter</code>.
On 2013/02/26 03:12:07, yanivi wrote:
> [optional] I don't like comments on constructors, unless there is something
> useful to say that isn't obvious from the parameters.
Done - I removed the documentation
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:71:
public void sawOpcode(int seen) {
I prefer not wrapping the whole switch case (although it's nice option and may
reduce code) because I prefer to limit the try and catch to contain as less code
as possible.
On 2013/02/26 03:12:07, yanivi wrote:
> [optional] you might consider wrapping this whole switch statement with a
> try-catch so you can catch ClassNotFoundException in one place instead of in
> getJavaClass and getSuperClass
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:77:
case INVOKEVIRTUAL: {
On 2013/02/26 03:12:07, yanivi wrote:
> [optional] I understand why you are using the { } block here, but I'm not
> necessarily a fan of this style. I think a simpler option would be to declare
> "JavaClass javaClass;" above the switch statement, and then remove the
> "JavaClass " from the javaClass declaration.
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:84:
if (method != null && isExperimental(method.getAnnotationEntries())) {
Changed as you suggested to checkMethod (and checkField also)
On 2013/02/26 03:12:07, yanivi wrote:
> why not just inline the implementation of getMethod into here? it is only
> called from one place.
>
> alternatively, since you're already creating a getMethod, you might as well
> rename it something like checkMethod and inline all of these lines into that
> method.
>
> similarly below for field
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:85:
bugReporter.reportBug(createBugInstance(EXPERIMENTAL_METHOD_USAGE).addCalledMethod(this));
On 2013/02/26 03:12:07, yanivi wrote:
> We shouldn't report a bug on an experimental method if we already reported a
bug
> on the experimental class. You could achieve that by returning null from
> getJavaClass if a bug was already reported.
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:136:
private boolean isExperimental(AnnotationEntry[] annotationEntries) {
On 2013/02/26 03:12:07, yanivi wrote:
> static
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:147:
* otherwise returns <code>null</code>.
On 2013/02/26 03:12:07, yanivi wrote:
> [optional] prefer {@code null}
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:184:
bugReporter.logError(
On 2013/02/26 03:12:07, yanivi wrote:
> surround with "if (!javaClass.isAbstract()) {"
Done. But I may need explanation why do you think it's important here.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:216:
if (isExperimental(javaClass.getAnnotationEntries())) {
On 2013/02/26 03:12:07, yanivi wrote:
> inline checkExperimentalClass into getJavaClass, wrapping it in "if (javaClass
> != null)
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
File
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/package-info.java
(right):
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/package-info.java:21:
*<code>
On 2013/02/26 03:12:07, yanivi wrote:
> remove <code> and </code>
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/package-info.java:31:
<version>${project.http.version}</version>
Agree. Don't see any other option
On 2013/02/26 03:12:07, yanivi wrote:
> [optional] this is a bit awkward, but I cannot think of a better option
either.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/package-info.java:40:
* @since 1.15
Why not just leaving it? It may be nice for users to know when it was added.
On 2013/02/26 03:12:07, yanivi wrote:
> [optional] I'm thinking that maybe we should remove @since because it's not
> really a direct part of the library.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
File google-http-client-findbugs/src/main/resources/findbugs.xml (right):
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/resources/findbugs.xml:1: <FindbugsPlugin
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
I added that file just in case we a user want to disable one of the errors by
setting its rank for more than 20. Although I don't think that's a common case.
Other than that I don't see any use of it.
But I added it anyway.
On 2013/02/26 03:12:07, yanivi wrote:
> do we need a bugrank.txt file?
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/resources/findbugs.xml:2:
xsi:noNamespaceSchemaLocation="findbugsplugin.xsd"
pluginid="edu.umd.cs.findbugs.pluginTutorial"
On 2013/02/26 03:12:07, yanivi wrote:
> please update pluginid, provider, and website
My bad, Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/resources/findbugs.xml:5: <Detector
class="com.google.api.client.findbugs.ExperimentalDetector"
I added speed="fast", explanation:
The required "speed" attribute supplies a value to be shown in the
"Settings->Configure Detectors" dialog. It gives the user an idea of how
expensive the analysis will be to perform. The value of this attribute should
be one of "fast", "moderate", or "slow"
No need for disabled
The optional "disabled" attribute, if set to "true", means that by default, the
detector will be disabled at runtime. This is useful for detectors that aren't
quite ready for prime time.
On 2013/02/26 03:12:07, yanivi wrote:
> do we need speed="fast" disabled="false"?
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/resources/findbugs.xml:7: <BugPattern
type="EXPERIMENTAL_CLASS_USAGE" abbrev="GAEU" category="CORRECTNESS" />
On 2013/02/26 03:12:07, yanivi wrote:
> category="BAD_PRACTICE"?
Done. 'BAD_PRACTICICE'
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
File google-http-client-findbugs/src/main/resources/messages.xml (right):
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/resources/messages.xml:14: <BugPattern
type="EXPERIMENTAL_CLASS_USAGE">
It appears on findbugs.xml, no need here.
On 2013/02/26 03:12:07, yanivi wrote:
> missing GAEU_ prefix?
>
> similarly below
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/resources/messages.xml:23: See <a
herf="https://code.google.com/p/google-http-java-client/source/browse/google-http-client/src/main/java/com/google/api/client/util/Experimental.java">@Experimental</a>
for more details
Can you please point me to the right place?
On 2013/02/26 03:12:07, yanivi wrote:
> This really ought to point to the JavaDoc, though we haven't published it yet.
> But we know already what the JavaDoc link would look like when it does.
https://codereview.appspot.com/7372049/diff/1/google-http-client/src/main/jav...
File
google-http-client/src/main/java/com/google/api/client/util/Experimental.java
(right):
https://codereview.appspot.com/7372049/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:22:
* subject to incompatible changes or removal.
On 2013/02/25 14:58:55, yanivi wrote:
> It's potentially even worse than that, because it may also mean that the
server
> side feature it depends on is subject to incompatible changes or removal at
any
> time. Developers need to read the JavaDoc of the API for details for a better
> understanding of the risk.
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:22:
* subject to incompatible changes or removal.
Done. I change most of the content, please review the whole content again, cause
my English isn't that good :)
On 2013/02/25 14:58:55, yanivi wrote:
> add "in a future release"
https://codereview.appspot.com/7372049/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:24:
* Note that the presence of this annotation implies nothing about the quality
or performance of
On 2013/02/25 14:58:55, yanivi wrote:
> Please remove this sentence. The presence of the "Experimental" annotation
may
> imply something about the quality or performance of the API in question. That
> may well have been the reason we marked it as Experimental.
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:25:
* the API in question, only the fact that it is not "API-frozen".
On 2013/02/25 14:58:55, yanivi wrote:
> Add a sentence that it is generally inadvisable for other non-experimental
> libraries to use experimental API from this library. The problem is that
other
> libraries don't have control over the version of this library being used in
> client applications, and if the wrong version of this library is used, it has
> the potential to break client applications.
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:25:
* the API in question, only the fact that it is not "API-frozen".
On 2013/02/25 14:58:55, yanivi wrote:
> should we mention the FindBugs plugin in the JavaDoc here?
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client/src/main/jav...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:34:
public @interface Experimental {
On 2013/02/25 14:58:55, yanivi wrote:
> please add @Documented annotation. See:
> http://docs.oracle.com/javase/7/docs/api/java/lang/annotation/Documented.html
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java File google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java (right): https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java#newcode38 google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:38: /** On 2013/02/27 15:32:38, peleyal wrote: > On 2013/02/26 ...
11 years, 1 month ago
(2013-02-27 17:10:10 UTC)
#4
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
File
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java
(right):
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:38:
/**
On 2013/02/27 15:32:38, peleyal wrote:
> On 2013/02/26 03:12:07, yanivi wrote:
> > [optional] For one-line JavaDoc I prefer the style:
> > /** Experimental annotation "signature". */
>
> Done.
[optional] Generally speaking, if I identify a problem in one place in the
changeset, please make the effort to check if you want to make the same change
elsewhere in the changeset. So in this case, you may want to shorten some of
the other JavaDoc comments.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:184:
bugReporter.logError(
On 2013/02/27 15:32:38, peleyal wrote:
> On 2013/02/26 03:12:07, yanivi wrote:
> > surround with "if (!javaClass.isAbstract()) {"
>
> Done. But I may need explanation why do you think it's important here.
Actually I don't know. I just looked at the implementation from Overstock and
they were doing that there. If you want to remove that you may.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
File
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/package-info.java
(right):
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/package-info.java:40:
* @since 1.15
On 2013/02/27 15:32:38, peleyal wrote:
> Why not just leaving it? It may be nice for users to know when it was added.
>
> On 2013/02/26 03:12:07, yanivi wrote:
> > [optional] I'm thinking that maybe we should remove @since because it's not
> > really a direct part of the library.
>
Well you removed it from ExperimentalDetector. I think the problem is precisely
that if we put @since 1.15 we are treating it like library code which this is
not, and we make no guarantees about its quality as a library, only as a tool.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
File google-http-client-findbugs/src/main/resources/messages.xml (right):
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/resources/messages.xml:23: See <a
herf="https://code.google.com/p/google-http-java-client/source/browse/google-http-client/src/main/java/com/google/api/client/util/Experimental.java">@Experimental</a>
for more details
On 2013/02/27 15:32:38, peleyal wrote:
> Can you please point me to the right place?
> On 2013/02/26 03:12:07, yanivi wrote:
> > This really ought to point to the JavaDoc, though we haven't published it
yet.
>
> > But we know already what the JavaDoc link would look like when it does.
>
probably it will be, though it hasn't been decided yet:
http://javadoc.google-http-java-client.googlecode.com/hg/1.15.0-rc/com/google...https://codereview.appspot.com/7372049/diff/28001/google-http-client-findbugs...
File
google-http-client-findbugs/google-http-client-findbugs-test/src/main/java/com/google/api/client/findbugs/test/Test.java
(right):
https://codereview.appspot.com/7372049/diff/28001/google-http-client-findbugs...
google-http-client-findbugs/google-http-client-findbugs-test/src/main/java/com/google/api/client/findbugs/test/Test.java:22:
public class Test {
[optional] It would be nice to also test if we suppress reporting an error if an
experimental class/method/field is used from an experimental class/method.
https://codereview.appspot.com/7372049/diff/28001/google-http-client-findbugs...
File
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java
(right):
https://codereview.appspot.com/7372049/diff/28001/google-http-client-findbugs...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:133:
private JavaClass checkClass() {
maybe add a TODO to check if caching the experimental state of every class could
improve performance on large projects?
https://codereview.appspot.com/7372049/diff/28001/google-http-client-findbugs...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:169:
private void checkMethod(JavaClass javaClass, String methodName, String
signature) {
Rather than passing javaClass as a parameter, you may just call checkClass()
from here directly. similarly for checkField. Simplifying your code should make
it easier to maintain.
https://codereview.appspot.com/7372049/diff/28001/google-http-client-findbugs...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:175:
bugReporter.reportBug(
design issue: is it possible to check if the experimental field/method/class is
being called from an experimental class/method? ideally we wouldn't report a bug
in such a case. It would allow us to use the FindBugs plugin on our own code
base to catch uses of Experimental code from non-Experimental code.
https://codereview.appspot.com/7372049/diff/28001/google-http-client-findbugs...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:199:
// field has been found - check if its' experimental
it's not its'
https://codereview.appspot.com/7372049/diff/28001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/util/Experimental.java
(right):
https://codereview.appspot.com/7372049/diff/28001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:23:
* incompatible changes or removal in the future. It may also mean that the
server's feature it
Proper JavaDoc style uses a singel sentence at the top that fully summarizes the
class, and then the rest of the content is found in follow-up paragraphs. So
the first paragraph must have only a single sentence.
Here we want the first sentence to also specify that it may mean that the server
features it depends on are potentially subject to breakage at any time.
https://codereview.appspot.com/7372049/diff/28001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:23:
* incompatible changes or removal in the future. It may also mean that the
server's feature it
server features, not server's features
https://codereview.appspot.com/7372049/diff/28001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:24:
* depends on, is as well a subject to incompatible changes or removal in the
future.
replace whole line with "depends on are potentially subject to breakage at any
time".
https://codereview.appspot.com/7372049/diff/28001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:27:
* An API bearing this annotation is exempt from any compatibility guarantees
made by its containing
specify "class, method, or field" like you did before
https://codereview.appspot.com/7372049/diff/28001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:29:
* of the risk.
Let's expand more about our general policy of how we handle incompatible changes
for API marked experimental:
To provide a smoother upgrade path when we make incompatible changes to
experimental API, whenever possible we try to deprecate the old experimental API
in the first minor release, and then remove it in the second minor release.
https://codereview.appspot.com/7372049/diff/28001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:34:
* this library.
The problem is that other libraries don't have control over the version of this
library being used in client applications, and if the wrong version of this
library is used, it has the potential to break client applications.
https://codereview.appspot.com/7372049/diff/28001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:38:
* You may use the google-http-client-findbugs Plugin to find usages of API
bearing this annotation.
Plugin -> plugin
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java File google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java (right): https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java#newcode184 google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:184: bugReporter.logError( I also saw that in their implementation, but ...
11 years, 1 month ago
(2013-02-28 16:57:08 UTC)
#5
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
File
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java
(right):
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:184:
bugReporter.logError(
I also saw that in their implementation, but didn't understand why we need it.
On 2013/02/27 17:10:10, yanivi wrote:
> On 2013/02/27 15:32:38, peleyal wrote:
> > On 2013/02/26 03:12:07, yanivi wrote:
> > > surround with "if (!javaClass.isAbstract()) {"
> >
> > Done. But I may need explanation why do you think it's important here.
>
> Actually I don't know. I just looked at the implementation from Overstock and
> they were doing that there. If you want to remove that you may.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
File
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/package-info.java
(right):
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/package-info.java:40:
* @since 1.15
On 2013/02/27 17:10:10, yanivi wrote:
> On 2013/02/27 15:32:38, peleyal wrote:
> > Why not just leaving it? It may be nice for users to know when it was added.
> >
> > On 2013/02/26 03:12:07, yanivi wrote:
> > > [optional] I'm thinking that maybe we should remove @since because it's
not
> > > really a direct part of the library.
> >
>
> Well you removed it from ExperimentalDetector. I think the problem is
precisely
> that if we put @since 1.15 we are treating it like library code which this is
> not, and we make no guarantees about its quality as a library, only as a tool.
Done.
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
File google-http-client-findbugs/src/main/resources/messages.xml (right):
https://codereview.appspot.com/7372049/diff/1/google-http-client-findbugs/src...
google-http-client-findbugs/src/main/resources/messages.xml:23: See <a
herf="https://code.google.com/p/google-http-java-client/source/browse/google-http-client/src/main/java/com/google/api/client/util/Experimental.java">@Experimental</a>
for more details
On 2013/02/27 17:10:10, yanivi wrote:
> On 2013/02/27 15:32:38, peleyal wrote:
> > Can you please point me to the right place?
> > On 2013/02/26 03:12:07, yanivi wrote:
> > > This really ought to point to the JavaDoc, though we haven't published it
> yet.
> >
> > > But we know already what the JavaDoc link would look like when it does.
> >
>
> probably it will be, though it hasn't been decided yet:
>
>
http://javadoc.google-http-java-client.googlecode.com/hg/1.15.0-rc/com/google...
Done.
https://codereview.appspot.com/7372049/diff/28001/google-http-client-findbugs...
File
google-http-client-findbugs/google-http-client-findbugs-test/src/main/java/com/google/api/client/findbugs/test/Test.java
(right):
https://codereview.appspot.com/7372049/diff/28001/google-http-client-findbugs...
google-http-client-findbugs/google-http-client-findbugs-test/src/main/java/com/google/api/client/findbugs/test/Test.java:22:
public class Test {
I improved the detector. It doesn't report on @Experimental usage inside
@Experimental class or method.
On 2013/02/27 17:10:10, yanivi wrote:
> [optional] It would be nice to also test if we suppress reporting an error if
an
> experimental class/method/field is used from an experimental class/method.
https://codereview.appspot.com/7372049/diff/28001/google-http-client-findbugs...
File
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java
(right):
https://codereview.appspot.com/7372049/diff/28001/google-http-client-findbugs...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:133:
private JavaClass checkClass() {
On 2013/02/27 17:10:10, yanivi wrote:
> maybe add a TODO to check if caching the experimental state of every class
could
> improve performance on large projects?
Done.
https://codereview.appspot.com/7372049/diff/28001/google-http-client-findbugs...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:169:
private void checkMethod(JavaClass javaClass, String methodName, String
signature) {
On 2013/02/27 17:10:10, yanivi wrote:
> Rather than passing javaClass as a parameter, you may just call checkClass()
> from here directly. similarly for checkField. Simplifying your code should
make
> it easier to maintain.
Done.
https://codereview.appspot.com/7372049/diff/28001/google-http-client-findbugs...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:175:
bugReporter.reportBug(
Solve that. That's part of the getClass method
On 2013/02/27 17:10:10, yanivi wrote:
> design issue: is it possible to check if the experimental field/method/class
is
> being called from an experimental class/method? ideally we wouldn't report a
bug
> in such a case. It would allow us to use the FindBugs plugin on our own code
> base to catch uses of Experimental code from non-Experimental code.
https://codereview.appspot.com/7372049/diff/28001/google-http-client-findbugs...
google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:199:
// field has been found - check if its' experimental
On 2013/02/27 17:10:10, yanivi wrote:
> it's not its'
Done.
https://codereview.appspot.com/7372049/diff/28001/google-http-client/src/main...
File
google-http-client/src/main/java/com/google/api/client/util/Experimental.java
(right):
https://codereview.appspot.com/7372049/diff/28001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:23:
* incompatible changes or removal in the future. It may also mean that the
server's feature it
On 2013/02/27 17:10:10, yanivi wrote:
> server features, not server's features
Done.
https://codereview.appspot.com/7372049/diff/28001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:24:
* depends on, is as well a subject to incompatible changes or removal in the
future.
On 2013/02/27 17:10:10, yanivi wrote:
> replace whole line with "depends on are potentially subject to breakage at any
> time".
Done.
https://codereview.appspot.com/7372049/diff/28001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:27:
* An API bearing this annotation is exempt from any compatibility guarantees
made by its containing
On 2013/02/27 17:10:10, yanivi wrote:
> specify "class, method, or field" like you did before
Done.
https://codereview.appspot.com/7372049/diff/28001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:34:
* this library.
On 2013/02/27 17:10:10, yanivi wrote:
> The problem is that other libraries don't have control over the version of
this
> library being used in client applications, and if the wrong version of this
> library is used, it has the potential to break client applications.
Done.
https://codereview.appspot.com/7372049/diff/28001/google-http-client/src/main...
google-http-client/src/main/java/com/google/api/client/util/Experimental.java:38:
* You may use the google-http-client-findbugs Plugin to find usages of API
bearing this annotation.
On 2013/02/27 17:10:10, yanivi wrote:
> Plugin -> plugin
Done.
LGTM Well done! https://codereview.appspot.com/7372049/diff/54001/google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java File google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java (right): https://codereview.appspot.com/7372049/diff/54001/google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java#newcode114 google-http-client-findbugs/src/main/java/com/google/api/client/findbugs/ExperimentalDetector.java:114: * Returns JavaClass for the current ...
11 years, 1 month ago
(2013-03-01 02:24:09 UTC)
#6
Issue 7372049: FindBugs plugin to catch uses of @Experimental annotation
(Closed)
Created 11 years, 2 months ago by peleyal
Modified 11 years, 1 month ago
Reviewers: yanivi
Base URL: https://code.google.com/p/google-http-java-client/
Comments: 113
