[GitHub] [beam] jkff commented on issue #11261: [BEAM-9147] Add a VideoIntelligence transform to Java SDK

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

jkff commented on issue #11261: [BEAM-9147] Add a VideoIntelligence transform to Java SDK
URL: https://github.com/apache/beam/pull/11261#issuecomment-615463060
 
 
   Hi! Drive-by comment: in the current form this transform violates https://beam.apache.org/contribute/ptransform-style-guide/#exposing-a-ptransform-vs-something-else , specifically, it exposes a set of DoFn's rather than exposing a set of PTransforms. There are several very compelling advantages to exposing PTransform's, mainly, compatibility: once it's a DoFn, you can never add anything else into it (e.g. a reshuffle) without breaking client code - but you can change the internal implementation of a PTransform at will. Many PTransforms that used to be DoFn's before that style guide was created, eventually evolved to be more complex.
   
   Could you consider restructuring it in a follow-up PR? A similar DoFn-like PTransform you can take example from is https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Regex.java.
   
   Thanks!

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services