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

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

jkff edited a comment 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 the [PTransform Style Guide](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 `PTransform`s.
   
   There are several very compelling advantages to exposing `PTransform`s, mainly, compatibility: once it's a `DoFn` and clients use it via `ParDo`, 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 `PTransform`s 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 [Regex](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