eric-maynard commented on code in PR #2007:
URL: https://github.com/apache/polaris/pull/2007#discussion_r2193169188
##########
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:
> Any java symbols inside polaris-core do not have backward-compatibility
guaranteed per https://polaris.apache.org/in-dev/unreleased/evolution/
This works against the argument to refactor, then -- the idea behind an
accessor method which just returns a private field is to add a layer of
abstraction to make changes _later_ without breaking an API, but if your
argument is that breaking the API is okay then I don't see an impetus to add an
accessor method at this time.
> Exposing direct field access outside of a class makes refactoring harder,
debugging harder, and reasoning about code behaviour harder... IMHO.
"Honestly"... I disagree. With private fields like this, there's a chance
that BehaviorChangeConfiguration and FeatureConfiguration can diverge from one
another and from PolarisConfiguration in unexpected ways. If nothing else the
fields should be protected. I don't see how a method call that returns a field
can possibly make debugging easier than just having the field be accessed.
--
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]
