Hi all, But aren't we now building on Java 11+? I think we could go one step ahead and replace most of these Guava factory methods by List.of(), List.copyOf() and the like – as long as the collection is not modified after. It's more concise and saves us a Guava import.
Thanks, Alex On Thu, Oct 24, 2024 at 7:21 PM rdb...@gmail.com <rdb...@gmail.com> wrote: > 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 >> >
