Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove duplicate entry in TypesWithUndefinedEquality#SPARSE_ARRAY #29

Closed
wants to merge 3 commits into from

Conversation

rickie
Copy link
Member

@rickie rickie commented Feb 17, 2022

While there:

  • Remove duplicate entry of TypesWithUndefinedEquality#SPARSE_ARRAY
  • Sort the enum
  • Sort the values of the enum

When opening the PR, add the following comment (open to suggestions):

Should we add java.lang.Object to the TypesWithUndefinedEquality enum?

@rickie rickie changed the title Introduce TypesWithUndefinedEquality#OBJECT Remove duplicate entry TypesWithUndefinedEquality#SPARSE_ARRAY Mar 2, 2022
@rickie rickie changed the title Remove duplicate entry TypesWithUndefinedEquality#SPARSE_ARRAY Remove duplicate entry in TypesWithUndefinedEquality#SPARSE_ARRAY Mar 2, 2022
@rickie rickie force-pushed the rossendrijver/object_undefined_equality branch from dde42c8 to f6db6b0 Compare March 2, 2022 16:12
@rickie
Copy link
Member Author

rickie commented Mar 2, 2022

To make things easier for them to review, ideally, we do something like we did here.

Perhaps @Stephan202 you have an idea on this?

The best I got was this:

comm -3 <(git show origin/master:core/src/main/java/com/google/errorprone/bugpatterns/TypesWithUndefinedEquality.java | awk '{$1=$1};1' | sort) <(git show origin/rossendrijver/object_undefined_equality:core/src/main/java/com/google/errorprone/bugpatterns/TypesWithUndefinedEquality.java | awk '{$1=$1};1' | sort )

With the somewhat confusing output 😄 :

"android.util.SparseArray",
"androidx.collection.LongSparseArrayCompat"),
	"androidx.collection.LongSparseArrayCompat",
"androidx.collection.SparseArrayCompat"),
"androidx.collection.SparseArrayCompat",
	"androidx.core.util.LongSparseArrayCompat"),
"androidx.core.util.LongSparseArrayCompat",
	DATE("Date", "java.util.Date"),
DATE("Date", "java.util.Date");
	ITERABLE("Iterable", "com.google.common.collect.FluentIterable", "java.lang.Iterable"),
ITERABLE("Iterable", "java.lang.Iterable", "com.google.common.collect.FluentIterable"),
"SparseArray",
SPARSE_ARRAY(
	SPARSE_ARRAY("SparseArray", "android.util.SparseArray", "androidx.collection.SparseArrayCompat");

@Stephan202 Stephan202 force-pushed the rossendrijver/object_undefined_equality branch from f6db6b0 to 37f5a40 Compare April 5, 2022 19:41
Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased and added a commit. The following (hacky, likely Linux-only) script verifies that this PR does not introduce additional changes:

#!/usr/bin/env bash

set -e -u -o pipefail

normalize() {
  local rev="${1}"

  git show "${rev}:core/src/main/java/com/google/errorprone/bugpatterns/TypesWithUndefinedEquality.java" \
    | grep -Pzo '(?<=LONG_SPARSE_ARRAY\()[^)]+' | grep -Pao '[\w.]+' | sort -u
  git show "${rev}:core/src/main/java/com/google/errorprone/bugpatterns/TypesWithUndefinedEquality.java" \
    | grep -Pzo '(?<=SPARSE_ARRAY\()[^)]+' | grep -Pao '[\w.]+' | sort -u
  git show "${rev}:core/src/main/java/com/google/errorprone/bugpatterns/TypesWithUndefinedEquality.java" \
    | grep -Po '[A-Z_]+\(' | sort -u
}

diff -u <(normalize master) <(normalize HEAD)

Suggested commit message:

Remove duplicate `TypesWithUndefinedEquality#SPARSE_ARRAY` entry

Type `androidx.collection.SparseArrayCompat` was listed twice.

While there, sort the enum elements and type names.

IMMUTABLE_MULTIMAP("ImmutableMultimap", "com.google.common.collect.ImmutableMultimap"),
IMMUTABLE_COLLECTION("ImmutableCollection", "com.google.common.collect.ImmutableCollection"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C comes before M 👀

@rickie rickie force-pushed the rossendrijver/object_undefined_equality branch from 37f5a40 to 7491fed Compare April 7, 2022 06:47
@rickie rickie force-pushed the rossendrijver/object_undefined_equality branch from 7491fed to c697984 Compare April 7, 2022 06:50
@rickie
Copy link
Member Author

rickie commented Apr 7, 2022

OK rebased it and squash some of the commits. Shall I open the PR with the suggested commit message as description and add a comment as suggested in the PR description?

Also, the script should be in the PR description then 😄 ?

@Stephan202
Copy link
Member

I'd open the PR with just the suggested commit message as text, and then add the script as a comment, I think :)

As for adding a comment about Object: yep, SGTM!

@rickie
Copy link
Member Author

rickie commented Apr 14, 2022

Opened PR upstream: google#3110

@rickie
Copy link
Member Author

rickie commented Apr 23, 2022

Fixed in: google@f35b9df.

@rickie rickie closed this Apr 23, 2022
@Stephan202 Stephan202 deleted the rossendrijver/object_undefined_equality branch April 24, 2022 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants