tanclary commented on code in PR #2750:
URL: https://github.com/apache/calcite/pull/2750#discussion_r1442342998
##########
testkit/src/main/java/org/apache/calcite/test/DiffRepository.java:
##########
@@ -877,7 +880,7 @@ public static DiffRepository lookup(Class<?> clazz,
return REPOSITORY_CACHE.getUnchecked(key);
}
- /**
+ /**
Review Comment:
mistake?
##########
testkit/src/test/java/org/apache/calcite/test/DiffRepositoryTest.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.test;
+
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+import org.junit.jupiter.api.Test;
+
+import java.io.File;
+import java.net.URL;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/** Tests DiffRepository for fixtures.
+ *
+ * @see DiffRepository */
+
+/**
+ * Stub of DiffRepository to create a fake source JAR file.
+ */
+class JarStubDiffRepository extends DiffRepository {
+ /**
+ * Creates a DiffRepository.
+ *
+ * @param refFile Reference file
+ * @param logFile Log file
+ * @param baseRepository Parent repository or null
+ * @param filter Filter or null
+ * @param indent Indentation of XML file
+ */
+ private JarStubDiffRepository(URL refFile, File logFile,
+ @Nullable DiffRepository baseRepository, Filter filter, int indent) {
+ super(refFile, logFile, baseRepository, filter, indent);
+ }
+
+ public static DiffRepository lookup(Class<?> clazz) {
+ final JarKey key = new JarKey(clazz, null, null, 2);
+ return REPOSITORY_CACHE.getUnchecked(key);
+ }
+ static final LoadingCache<JarKey, DiffRepository> REPOSITORY_CACHE =
+ CacheBuilder.newBuilder().build(CacheLoader.from(JarKey::toRepo));
+ private static class JarKey extends Key {
+ JarKey(Class<?> clazz, DiffRepository baseRepository, Filter filter, int
indent) {
+ super(clazz, baseRepository, filter, indent);
+ // STUBS THE REF FILE TO PRETEND IT LIVES IN A JAR
+ this.refFilePath = "mypath/calcite-stub.jar!/randompath/abcdef.xml";;
+ }
+ }
+}
+public class DiffRepositoryTest {
+
+ /**
+ * Test that a DiffRepository from a file in a JAR
+ * would create a log file outside of the JAR.
+ **/
+ @Test void testLogFilePathInJar() {
+ final DiffRepository diffRepo =
JarStubDiffRepository.lookup(FixtureTest.class);
Review Comment:
Looks so much better now
##########
core/src/test/java/org/apache/calcite/util/SourceTest.java:
##########
@@ -118,6 +120,22 @@ void testAbsoluteFileToUrl(String path, String
expectedUrl) throws URISyntaxExce
() -> "Sources.of(Sources.of(file(" + path +
").absolutePath).url()).file().getPath()");
}
+ /**
+ * [CALCITE-5052] Resources created with the JAR protocol should also work.
Review Comment:
if youre going to link a jira case you can format it like this example:
https://github.com/apache/calcite/blob/main/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java#L277
##########
testkit/src/main/java/org/apache/calcite/test/DiffRepository.java:
##########
@@ -900,18 +903,21 @@ String filter(
}
/** Cache key. */
- private static class Key {
+ protected static class Key {
private final Class<?> clazz;
private final DiffRepository baseRepository;
private final Filter filter;
private final int indent;
-
+ private URL refFile;
+ String refFilePath;
Review Comment:
To be honest I like the newline after all the variables and before the
`Key() {}`
##########
core/src/test/java/org/apache/calcite/util/SourceTest.java:
##########
@@ -40,7 +42,7 @@
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.hasToString;
+import static org.hamcrest.Matchers.*;
Review Comment:
I think Calcite uses explicit imports rather than .*; this is configurable
in intellij settings
https://www.jetbrains.com/help/idea/creating-and-optimizing-imports.html#disable-wildcard-imports
##########
testkit/src/test/java/org/apache/calcite/test/DiffRepositoryTest.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.test;
+
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+import org.junit.jupiter.api.Test;
+
+import java.io.File;
+import java.net.URL;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/** Tests DiffRepository for fixtures.
+ *
+ * @see DiffRepository */
+
+/**
+ * Stub of DiffRepository to create a fake source JAR file.
+ */
+class JarStubDiffRepository extends DiffRepository {
+ /**
+ * Creates a DiffRepository.
+ *
+ * @param refFile Reference file
+ * @param logFile Log file
+ * @param baseRepository Parent repository or null
+ * @param filter Filter or null
+ * @param indent Indentation of XML file
+ */
+ private JarStubDiffRepository(URL refFile, File logFile,
+ @Nullable DiffRepository baseRepository, Filter filter, int indent) {
+ super(refFile, logFile, baseRepository, filter, indent);
+ }
+
+ public static DiffRepository lookup(Class<?> clazz) {
+ final JarKey key = new JarKey(clazz, null, null, 2);
Review Comment:
should these have comments // be spaced out ? I think the actual logic looks
fine !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
