It’s correct that these methods aren’t strictly needed. We could translate every case into a slightly different form:
Lists.newArrayList() -> new ArrayList<>() Lists.newArrayList(iter) -> new ArrayList(); Iterators.addAll(list, iter) Lists.newArrayList(iterable) -> new ArrayList<>(); Iterators.addAll(list, iterable.iterator() Lists.newArrayList(collection) -> new ArrayList<>(collection) Lists.newArrayList(1, 2, 3) -> new ArrayList<>(); Collections.addAll(list, 1, 2, 3) You could make that argument for all syntactic sugar. These constructors are concise, clear, and avoid a bit of code that’s specific to the thing you’re converting to a list. That’s why this is part of the project’s style. I think there’s value here, but even if this were an arbitrary choice I don’t think there’s value in refactoring to avoid it. If and when these convenience methods are deprecated, we can always replace them with our own utility in iceberg-common and move on without a ton of changes. Ryan On Thu, Oct 24, 2024 at 9:58 AM Jean-Baptiste Onofré <j...@nanthrax.net> wrote: > Hi Eduard > > Yeah, I mean checkstyle (not spotless). > > AFAIR, I saw a couple of locations without the diamond syntax. Let me > find it out. Maybe we can start with fixing there. > > Thanks ! > Regards > JB > > On Thu, Oct 24, 2024 at 5:07 PM Eduard Tudenhöfner > <etudenhoef...@apache.org> wrote: > > > > Hey JB, > > > > I don't think we're ever using e.g. Lists.newArrayList() without the > diamond syntax in the codebase, so it's typically always List<String> list > = Lists.newArrayList(). > > So I wonder how much of an issue that actually is? Do you have examples > in the codebase that don't use the diamond syntax and is it worth updating > the entire codebase? > > > >> to loosen the Lists/Maps/... enforcement in spotless) > > > > > > FYI this is being enforced by checkstyle and was first introduced by > https://github.com/apache/iceberg/pull/3689 in order to have a consistent > style around collection instantiation. > > > > Thanks > > Eduard > > > > On Thu, Oct 24, 2024 at 8:20 AM Jean-Baptiste Onofré <j...@nanthrax.net> > wrote: > >> > >> Hi folks, > >> > >> We are using Guava for different "utils" methods. Especially, we are > >> using Guava to create lists and maps. For instance, we do (and we > >> force the use of): > >> > >> List myList = Lists.newArrayList(); > >> > >> or > >> > >> Map myMap = Maps.newHashMap(); > >> > >> If it was a good idea up to JDK7, these methods are now unnecessary, > >> and the "regular" approach should be preferred in order to cleanly use > >> the diamond syntax > >> ( > https://docs.oracle.com/javase/tutorial/java/generics/genTypeInference.html#type-inference-instantiation > ). > >> > >> The Guava Javadoc actually clearly states this: > >> > https://github.com/google/guava/blob/v33.3.1/guava/src/com/google/common/collect/Lists.java#L79 > >> > >> So, I propose: > >> - to loosen the Lists/Maps/... enforcement in spotless) > >> - to refactore Iceberg to use JDK list and map constructors instead of > >> Guava Lists/Maps/... > >> > >> I started the change, but as it's a pretty large one, I would like > >> your feedback before completing the PR. > >> > >> Regards > >> JB >
