increase severity of multiple errorprone checks #6800
Conversation
XN137
force-pushed
the
more-errorprone-errors
branch
7 times, most recently
from
f121915 to
09c1871
Compare
|
Running |
note that we fix DifferentNameButSame with a minimal diff, but not make it uniform across all files
XN137
force-pushed
the
more-errorprone-errors
branch
from
09c1871 to
8dbe771
Compare
...bility-tests/src/intTest/java/org/projectnessie/tools/compatibility/tests/ITUpgradePath.java
Outdated
Show resolved
Hide resolved
| @@ -51,7 +50,7 @@ public Status status() { | |||
|
|
|||
| public abstract String filePath(); | |||
|
|
|||
| public abstract StructType partitionType(); | |||
| public abstract Types.StructType partitionType(); | |||
There was a problem hiding this comment.
Why do we sometimes import the inner class name, and sometime qualify it with its parent class name?
In this particular case, the inner class name seems to be non-ambiguous, so why do we change it to be qualified by the parent type?
There was a problem hiding this comment.
as stated in the PR description we resolve DifferentNameButSame violations by doing the minimal amount of changes... so this file uses more Types.StructType than StructType right now, we we prefer to use the former for the fix.
if we can come up with a uniform convention on how to handle these things, we can also apply that kind of fix.
imo the first step is making things uniform on a per file basis, wdyt?
There was a problem hiding this comment.
IMHO, shorter type references (i.e. importing the most specific name possible) are preferable over smaller PR diff.
I'm open to other opinions too.
There was a problem hiding this comment.
I prefer shorter names as well, as long as they're not "common" names (there's already an errorprone check for common names like Builder).
XN137
force-pushed
the
more-errorprone-errors
branch
from
ce8c486 to
6638e7c
Compare
| @@ -1440,7 +1439,8 @@ public void fetchEntriesByNamelessReference() throws BaseNessieClientServerExcep | |||
| .operation(Put.of(b, tb)) | |||
| .commitMeta(CommitMeta.fromMessage("commit 1")) | |||
| .commit(); | |||
| List<Entry> entries = api().getEntries().hashOnRef(branch.getHash()).get().getEntries(); | |||
| List<EntriesResponse.Entry> entries = | |||
There was a problem hiding this comment.
is Entry too generic same as Builder i would have guessed yes, so using the more qualified name
There was a problem hiding this comment.
In the context of Nessie API (implied by this class), I think Entry has a specific meaning. I'd prefer keeping the import for it.
There was a problem hiding this comment.
As mentioned, errorprone's BadImport rule applies.
| @@ -34,14 +34,13 @@ | |||
| import org.projectnessie.versioned.persist.adapter.RefLog; | |||
| import org.projectnessie.versioned.persist.adapter.RepoDescription; | |||
| import org.projectnessie.versioned.persist.serialize.AdapterTypes; | |||
There was a problem hiding this comment.
all types in this file go through AdapterTypes it feels wrong to use the shorter variant only for RepoProps
...-extensions-basetests/src/main/java/org/projectnessie/spark/extensions/SparkSqlTestBase.java
Show resolved
Hide resolved
| @@ -1673,7 +1672,7 @@ private RefLogEntry writeRefLogEntry( | |||
| NamedRef ref, | |||
| RefLogHead refLogHead, | |||
| Hash commitHash, | |||
| Operation operation, | |||
| RefLogEntry.Operation operation, | |||
There was a problem hiding this comment.
should we prefer Operation in this file instead?
There was a problem hiding this comment.
I think in this particular case using the longer name is justified to identify this as belonging to the deprecated ref log functionality.
note that we fix DifferentNameButSame with a minimal diff, but
not make it uniform across all files