[GitHub] [beam] nbali commented on a diff in pull request #17775: [BEAM-14525] Fix for Protobuf getter/setter method name discovery issue

[GitBox <gi...@apache.org>]

nbali commented on code in PR #17775:
URL: https://github.com/apache/beam/pull/17775#discussion_r891082388


##########
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoByteBuddyUtils.java:
##########
@@ -190,15 +191,59 @@ class ProtoByteBuddyUtils {
   private static final String DEFAULT_PROTO_GETTER_PREFIX = "get";
   private static final String DEFAULT_PROTO_SETTER_PREFIX = "set";
 
+  // BEAM-14525: there is a slight difference between 'protoc' and Guava CaseFormat regarding the
+  // camel case conversion
+  // - guava keeps the first character after a number lower case
+  // - protoc makes it upper case
+  // based on
+  // https://github.com/protocolbuffers/protobuf/blob/ec79d0d328c7e6cea15cc27fbeb9b018ca289590/src/google/protobuf/compiler/java/helpers.cc#L173-L208
+  @VisibleForTesting
+  static String convertProtoPropertyNameToJavaPropertyName(String input) {

Review Comment:
   I originally went a name similar to this, but decided in favor of another for multiple reasons:
   - there seems to be no standard regarding conversion between various case formats - therefore this name isn't necessarily accurate -
   - it's not even guaranteed that the proto property is in lower underscore (foo2bar should be foo_2_bar IMO)
   - it's also not determined if foo2bar should be Foo2Bar or Foo2bar, there are converters for both cases
   
   I prefer to name methods as accurately as possible according to their behaviour and not regarding an implementation detail. The method name I proposed is accurate. The fact that the conversion (almost looks like or) is a lower underscore to upper camel case is an implementation detail and irrelevant. Also as it's not a standard can't be 100% accurate. Especially that ignores any special (non-alphanumeric) characters I really doubt thats how that conversion should work - so IMO it would be a bad idea for anyone to call it who needs a lower underscore to upper camel conversion.



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org