Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build warning fixes #75

Merged
merged 22 commits into from
Aug 6, 2024
Merged

Build warning fixes #75

merged 22 commits into from
Aug 6, 2024

Conversation

adutra
Copy link
Contributor

@adutra adutra commented Aug 2, 2024

Fixes a few build warnings. There is no functional change, and each commit can be reviewed separately.

@adutra adutra requested a review from a team as a code owner August 2, 2024 13:29
@@ -36,8 +36,7 @@ public class PolarisCatalogHelpers {
private PolarisCatalogHelpers() {}

public static List<String> tableIdentifierToList(TableIdentifier identifier) {
List<String> fullList = new ArrayList<>();
fullList.addAll(Arrays.asList(identifier.namespace().levels()));
List<String> fullList = new ArrayList<>(Arrays.asList(identifier.namespace().levels()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(optional) I don't think returning a mutable list is a good idea. I would recommend using ImmutableList of Guava as much as possible.

Copy link
Contributor

@sfc-gh-mcollado sfc-gh-mcollado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are great! Just the one question about the constant

public static final String REQUEST_PATH_CATALOG_INSTANCE_KEY = "REQUEST_PATH_CATALOG_INSTANCE";
String REQUEST_PATH_CATALOG_INSTANCE_KEY = "REQUEST_PATH_CATALOG_INSTANCE";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this no longer a constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still is: this is an interface so public static final is implicit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dur! 🤦🏽‍♂️

@flyrain flyrain closed this in #91 Aug 6, 2024
@snazy snazy reopened this Aug 6, 2024
@snazy
Copy link
Member

snazy commented Aug 6, 2024

(Reopening, was closed due to GH's "fix comment" confusion)

@adutra adutra merged commit 353acea into apache:main Aug 6, 2024
6 checks passed
@adutra adutra deleted the java-code-fixes branch August 6, 2024 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants