Hi, That was my idea: why not leveraging the JDK11 style here as we are now based on JDK11+ ?
Thoughts ? Regards JB On Thu, Oct 24, 2024 at 7:50 PM Alex Dutra <alex.du...@dremio.com.invalid> wrote: > > 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
