You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "gfakbar20 (via GitHub)" <gi...@apache.org> on 2025/06/07 04:43:24 UTC
[PR] Add checkstyle to detect star import [polaris]
gfakbar20 opened a new pull request, #1831:
URL: https://github.com/apache/polaris/pull/1831
<!--
Possible security vulnerabilities: STOP here and contact security@apache.org instead!
Please update the title of the PR with a meaningful message - do not leave it "empty" or "generated"
Please update this summary field:
The summary should cover these topics, if applicable:
* the motivation for the change
* a description of the status quo, for example the current behavior
* the desired behavior
* etc
PR checklist:
- Do a self-review of your code before opening a pull request
- Make sure that there's good test coverage for the changes included in this PR
- Run tests locally before pushing a PR (./gradlew check)
- Code should have comments where applicable. Particularly hard-to-understand
areas deserve good in-line documentation.
- Include changes and enhancements to the documentation (in site/content/in-dev/unreleased)
- For Work In Progress Pull Requests, please use the Draft PR feature.
Make sure to add the information BELOW this comment.
Everything in this comment will NOT be added to the PR description.
-->
This pr is made to solve #1100, based on #[@snazy suggestion](https://github.com/apache/polaris/issues/1100#issuecomment-2741501199).
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle to detect star import [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1831:
URL: https://github.com/apache/polaris/pull/1831#discussion_r2135776614
##########
codestyle/checkstyle-config.xml:
##########
@@ -0,0 +1,162 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE module PUBLIC
+ "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
+ "https://checkstyle.org/dtds/configuration_1_3.dtd">
+<!--
+ ~ 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.
+ -->
+<module name = "Checker">
+ <property name="charset" value="UTF-8"/>
+
+ <property name="severity" value="warning"/>
+
+ <property name="fileExtensions" value="java, properties, xml"/>
+ <!-- Excludes all 'module-info.java' files -->
+ <!-- See https://checkstyle.org/config_filefilters.html -->
+ <module name="BeforeExecutionExclusionFileFilter">
+ <property name="fileNamePattern" value="module\-info\.java$"/>
+ </module>
+ <!-- https://checkstyle.org/config_filters.html#SuppressionFilter -->
+ <module name="SuppressionFilter">
+ <property name="file" value="${org.checkstyle.google.suppressionfilter.config}"
+ default="checkstyle-suppressions.xml" />
+ <property name="optional" value="true"/>
+ </module>
+
+ <!-- Checks for whitespace -->
+ <!-- See http://checkstyle.org/config_whitespace.html -->
+ <module name="FileTabCharacter">
+ <property name="eachLine" value="true"/>
+ </module>
+
+ <module name="NewlineAtEndOfFileCheck"/>
+ <module name="SuppressWarningsFilter"/>
+
+ <module name="TreeWalker">
+ <module name="SuppressWarningsHolder" />
+ <module name="OuterTypeFilename"/>
+ <module name="IllegalTokenText">
+ <property name="tokens" value="STRING_LITERAL, CHAR_LITERAL"/>
+ <property name="format"
+ value="\\u00(09|0(a|A)|0(c|C)|0(d|D)|22|27|5(C|c))|\\(0(10|11|12|14|15|42|47)|134)"/>
+ <property name="message"
+ value="Consider using special escape sequence instead of octal value or Unicode escaped value."/>
+ </module>
+ <module name="AvoidEscapedUnicodeCharacters">
+ <property name="allowEscapesForControlCharacters" value="true"/>
+ <property name="allowByTailComment" value="true"/>
+ <property name="allowNonPrintableEscapes" value="true"/>
+ </module>
+ <module name="AvoidStarImport"/>
+ <module name="OneTopLevelClass"/>
+ <module name="NoLineWrap">
+ <property name="tokens" value="PACKAGE_DEF, IMPORT, STATIC_IMPORT"/>
+ </module>
+ <module name="OneStatementPerLine"/>
+ <module name="MultipleVariableDeclarations"/>
+ <module name="MissingSwitchDefault"/>
+ <module name="FallThrough"/>
+ <module name="PackageName">
+ <property name="format" value="^[a-z]+(\.[a-z][a-z0-9]*)*$"/>
+ <message key="name.invalidPattern"
+ value="Package name ''{0}'' must match pattern ''{1}''."/>
+ </module>
+ <module name="TypeName">
+ <property name="tokens" value="CLASS_DEF, INTERFACE_DEF, ENUM_DEF, ANNOTATION_DEF"/>
+ <message key="name.invalidPattern"
+ value="Type name ''{0}'' must match pattern ''{1}''."/>
+ </module>
+ <module name="MemberName">
+ <property name="format" value="^[a-z][a-z0-9][a-zA-Z0-9]*$"/>
+ <message key="name.invalidPattern"
+ value="Member name ''{0}'' must match pattern ''{1}''."/>
+ </module>
+ <module name="ParameterName">
+ <property name="format" value="^[a-z]([a-z0-9][a-zA-Z0-9]*)?$"/>
+ <message key="name.invalidPattern"
+ value="Parameter name ''{0}'' must match pattern ''{1}''."/>
+ </module>
+ <module name="LambdaParameterName">
+ <property name="format" value="^[a-z]([a-z0-9][a-zA-Z0-9]*)?$"/>
+ <message key="name.invalidPattern"
+ value="Lambda parameter name ''{0}'' must match pattern ''{1}''."/>
+ </module>
+ <module name="CatchParameterName">
+ <property name="format" value="^[a-z]([a-z0-9][a-zA-Z0-9]*)?$"/>
+ <message key="name.invalidPattern"
+ value="Catch parameter name ''{0}'' must match pattern ''{1}''."/>
+ </module>
+ <module name="LocalVariableName">
+ <property name="format" value="^[a-z]([a-z0-9][a-zA-Z0-9]*)?$"/>
Review Comment:
These checks apparently go way beyond start imports. I believe these ought to be discussed on the dev ML.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle to detect star import [polaris]
Posted by "gfakbar20 (via GitHub)" <gi...@apache.org>.
gfakbar20 commented on code in PR #1831:
URL: https://github.com/apache/polaris/pull/1831#discussion_r2133400149
##########
tools/version/build.gradle.kts:
##########
@@ -32,7 +32,9 @@ description =
val syncNoticeAndLicense by
tasks.registering(Sync::class) {
// Have to manually declare the inputs of this task here on top of the from/include below
- inputs.files(rootProject.layout.files("NOTICE*", "LICENSE*", "version.txt"))
+ inputs.files(fileTree(rootProject.rootDir) {
+ include("NOTICE*", "LICENSE*", "version.txt")
+ })
Review Comment:
So, after running `checkstyle` task, there are several task that follows like this (`syncNoticeAndLicense`) and `openApiGenerate`. What is the expected behavior? are those supposed to run after checkstyle or no? From my local, `syncNoticeAndLicense` always failed running. If it not happened on other device and not needed for checkstyle task in polaris, I will remove this and try to fix the checkstyle plugin config
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle to detect star import [polaris]
Posted by "gfakbar20 (via GitHub)" <gi...@apache.org>.
gfakbar20 commented on code in PR #1831:
URL: https://github.com/apache/polaris/pull/1831#discussion_r2136853769
##########
codestyle/checkstyle-config.xml:
##########
@@ -0,0 +1,162 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE module PUBLIC
+ "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
+ "https://checkstyle.org/dtds/configuration_1_3.dtd">
+<!--
+ ~ 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.
+ -->
+<module name = "Checker">
+ <property name="charset" value="UTF-8"/>
+
+ <property name="severity" value="warning"/>
+
+ <property name="fileExtensions" value="java, properties, xml"/>
+ <!-- Excludes all 'module-info.java' files -->
+ <!-- See https://checkstyle.org/config_filefilters.html -->
+ <module name="BeforeExecutionExclusionFileFilter">
+ <property name="fileNamePattern" value="module\-info\.java$"/>
+ </module>
+ <!-- https://checkstyle.org/config_filters.html#SuppressionFilter -->
+ <module name="SuppressionFilter">
+ <property name="file" value="${org.checkstyle.google.suppressionfilter.config}"
+ default="checkstyle-suppressions.xml" />
+ <property name="optional" value="true"/>
+ </module>
+
+ <!-- Checks for whitespace -->
+ <!-- See http://checkstyle.org/config_whitespace.html -->
+ <module name="FileTabCharacter">
+ <property name="eachLine" value="true"/>
+ </module>
+
+ <module name="NewlineAtEndOfFileCheck"/>
+ <module name="SuppressWarningsFilter"/>
+
+ <module name="TreeWalker">
+ <module name="SuppressWarningsHolder" />
+ <module name="OuterTypeFilename"/>
+ <module name="IllegalTokenText">
+ <property name="tokens" value="STRING_LITERAL, CHAR_LITERAL"/>
+ <property name="format"
+ value="\\u00(09|0(a|A)|0(c|C)|0(d|D)|22|27|5(C|c))|\\(0(10|11|12|14|15|42|47)|134)"/>
+ <property name="message"
+ value="Consider using special escape sequence instead of octal value or Unicode escaped value."/>
+ </module>
+ <module name="AvoidEscapedUnicodeCharacters">
+ <property name="allowEscapesForControlCharacters" value="true"/>
+ <property name="allowByTailComment" value="true"/>
+ <property name="allowNonPrintableEscapes" value="true"/>
+ </module>
+ <module name="AvoidStarImport"/>
+ <module name="OneTopLevelClass"/>
+ <module name="NoLineWrap">
+ <property name="tokens" value="PACKAGE_DEF, IMPORT, STATIC_IMPORT"/>
+ </module>
+ <module name="OneStatementPerLine"/>
+ <module name="MultipleVariableDeclarations"/>
+ <module name="MissingSwitchDefault"/>
+ <module name="FallThrough"/>
+ <module name="PackageName">
+ <property name="format" value="^[a-z]+(\.[a-z][a-z0-9]*)*$"/>
+ <message key="name.invalidPattern"
+ value="Package name ''{0}'' must match pattern ''{1}''."/>
+ </module>
+ <module name="TypeName">
+ <property name="tokens" value="CLASS_DEF, INTERFACE_DEF, ENUM_DEF, ANNOTATION_DEF"/>
+ <message key="name.invalidPattern"
+ value="Type name ''{0}'' must match pattern ''{1}''."/>
+ </module>
+ <module name="MemberName">
+ <property name="format" value="^[a-z][a-z0-9][a-zA-Z0-9]*$"/>
+ <message key="name.invalidPattern"
+ value="Member name ''{0}'' must match pattern ''{1}''."/>
+ </module>
+ <module name="ParameterName">
+ <property name="format" value="^[a-z]([a-z0-9][a-zA-Z0-9]*)?$"/>
+ <message key="name.invalidPattern"
+ value="Parameter name ''{0}'' must match pattern ''{1}''."/>
+ </module>
+ <module name="LambdaParameterName">
+ <property name="format" value="^[a-z]([a-z0-9][a-zA-Z0-9]*)?$"/>
+ <message key="name.invalidPattern"
+ value="Lambda parameter name ''{0}'' must match pattern ''{1}''."/>
+ </module>
+ <module name="CatchParameterName">
+ <property name="format" value="^[a-z]([a-z0-9][a-zA-Z0-9]*)?$"/>
+ <message key="name.invalidPattern"
+ value="Catch parameter name ''{0}'' must match pattern ''{1}''."/>
+ </module>
+ <module name="LocalVariableName">
+ <property name="format" value="^[a-z]([a-z0-9][a-zA-Z0-9]*)?$"/>
Review Comment:
Okay, i will follow this up to the mailing list.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle to detect star import [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #1831:
URL: https://github.com/apache/polaris/pull/1831#discussion_r2133367968
##########
tools/version/build.gradle.kts:
##########
@@ -32,7 +32,9 @@ description =
val syncNoticeAndLicense by
tasks.registering(Sync::class) {
// Have to manually declare the inputs of this task here on top of the from/include below
- inputs.files(rootProject.layout.files("NOTICE*", "LICENSE*", "version.txt"))
+ inputs.files(fileTree(rootProject.rootDir) {
+ include("NOTICE*", "LICENSE*", "version.txt")
+ })
Review Comment:
Do we need this change?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle to detect star import [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #1831:
URL: https://github.com/apache/polaris/pull/1831#issuecomment-2960022270
> Why not add the corresponding errorprone rule?
Agreed with @snazy, it would be a matter of just adding this snippet to `codestyle/errorprone-rules.properties`:
```
WildcardImport=ERROR
# Wildcard imports, static or otherwise, should not be used
```
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removing star import and adding errorprone rule [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #1831:
URL: https://github.com/apache/polaris/pull/1831#discussion_r2140732691
##########
gradle/libs.versions.toml:
##########
@@ -20,9 +20,9 @@
[versions]
hadoop = "3.4.1"
iceberg = "1.9.0" # Ensure to update the iceberg version in regtests to keep regtests up-to-date
-quarkus = "3.21.4"
immutables = "2.10.1"
picocli = "4.7.7"
+quarkus = "3.21.4"
Review Comment:
unrelated?
##########
codestyle/errorprone-rules.properties:
##########
@@ -294,3 +294,6 @@ UnnecessaryBoxedVariable=ERROR
UseEnumSwitch=ERROR
# Prefer using a switch instead of a chained if-else for enums
+
+WildcardImport=ERROR
+# Wildcard imports, static or otherwise, should not be used
Review Comment:
Super-nit: add a trailing LF
##########
plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/SparkCatalog.java:
##########
@@ -112,7 +112,7 @@ public CaseInsensitiveStringMap validateAndResolveCatalogOptions(
* Initialize REST Catalog for Iceberg and Polaris, this is the only catalog type supported by
* Polaris at this moment.
*/
- private void initRESTCatalog(String name, CaseInsensitiveStringMap options) {
+ private void initRestCatalog(String name, CaseInsensitiveStringMap options) {
Review Comment:
unrelated to the PR
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Removing star import and adding errorprone rule [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy merged PR #1831:
URL: https://github.com/apache/polaris/pull/1831
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle to detect star import [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1831:
URL: https://github.com/apache/polaris/pull/1831#discussion_r2135773211
##########
plugins/spark/v3.5/spark/src/main/java/org/apache/polaris/spark/rest/LoadGenericTableRestResponse.java:
##########
@@ -29,10 +29,10 @@
* currently required because the Iceberg HTTPClient requires the request and response to be a class
* of RESTRequest and RESTResponse.
*/
-public class LoadGenericTableRESTResponse extends LoadGenericTableResponse implements RESTResponse {
+public class LoadGenericTableRestResponse extends LoadGenericTableResponse implements RESTResponse {
@JsonCreator
- public LoadGenericTableRESTResponse(
+ public LoadGenericTableRestResponse(
Review Comment:
This kind of style change need to be discussed on dev@polaris.apache.org, please.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle to detect star import [polaris]
Posted by "gfakbar20 (via GitHub)" <gi...@apache.org>.
gfakbar20 commented on PR #1831:
URL: https://github.com/apache/polaris/pull/1831#issuecomment-2961130260
i will fix it tonight
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add checkstyle to detect star import [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #1831:
URL: https://github.com/apache/polaris/pull/1831#issuecomment-2960151394
+1 to enforcing this in ErrorProne
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org