poojanilangekar commented on code in PR #2007:
URL: https://github.com/apache/polaris/pull/2007#discussion_r2192861661
##########
polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java:
##########
@@ -124,6 +124,18 @@ T cast(Object value) {
return this.typ.cast(value);
}
+ public String key() {
+ return key;
+ }
+
+ public String description() {
+ return description;
+ }
+
+ public T defaultValue() {
+ return defaultValue;
+ }
Review Comment:
Adding @dimas-b's comment here, (and let him address your question):
> Sorry, I did not notice this before, but I think having public fiends in
general is not a good idea. Since you're toughing this code in current PR,
would you mind following up with another PR to convert them to private fields?
I agree that it's probably a bad idea to have public member variables, but
don't feel strongly either way.
The other part that I'd be concerned about is the inconsistent
public/private handling of fields. Since the fields are all `final`, why
shouldn't `catalogConfigImpl`, `catalogConfigUnsafeImpl`, and `typ` be `public`?
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
