ddanielr commented on code in PR #3173:
URL: https://github.com/apache/accumulo/pull/3173#discussion_r1088250767
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -1702,16 +1702,20 @@ private static <T extends Annotation> boolean
hasPrefixWithAnnotation(String key
/**
* Checks if the given property and value are valid. A property is valid if
the property key is
- * valid see {@link #isValidTablePropertyKey} and that the value is a valid
format for the type
- * see {@link PropertyType#isValidFormat}.
+ * valid see {@link #isValidPropertyKey} and that the value is a valid
format for the type see
+ * {@link PropertyType#isValidFormat}.
*
* @param key property key
* @param value property value
* @return true if key is valid (recognized, or has a recognized prefix)
*/
- public static boolean isTablePropertyValid(final String key, final String
value) {
+ public static boolean isValidProperty(final String key, final String value) {
Review Comment:
I agree that the previous method was constrained to only checking table
validity.
It was accomplishing this by checking if the property was valid and then
also doing a call to `isValidTablePropertyKey` to determine if the property key
itself was valid for tables.
However, since `isValidTablePropertyKey` is public, I refactored all usages
of `isTablePropertyValid` with two separate method calls to `isValidProperty`
and `isValidTablePropertyKey`.
I did this because I wanted to try and streamline the `Property.java` class.
This class had this method (`isTablePropertyValid`) along with
`isValidTablePropertyKey` and `isValidBooleanPropertyKey`.
If we followed this pattern, we would end up with a validity check for each
property type
[[PropertyType.java](https://github.com/ddanielr/accumulo/blob/feature/thrift-woes/core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java)
as well as validity checks for table, namespace, and system key types.
The end state would be a cluttered class with a lot of different
`isValid<X>` methods that has the potential to be confusing.
I chose to streamline down to a single `isValid` method and enforce the
context check of "table" in the usage location.
However, if those extra methods are the desired goal then I can pivot and
create a new generic `isValidProperty` method while leaving
`isTablePropertyValid` intact?
--
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]
