@dan I thought you were talking about the transform class definition:
public static class GroupedValues<K, InputT, OutputT>
extends PTransform
<PCollection<? extends KV<K, ? extends
Iterable<InputT>>>,
PCollection<KV<K, OutputT>>> {
On Fri, Jan 27, 2017 at 11:30 AM Dan Halperin <[email protected]>
wrote:
> Hi Jesse, can you specifically say which functions on Combine and Count
> you're thinking of? I believe these transforms are consistent with the
> "principle of least visibility" -- make nothing more public than it needs
> to be.
>
> Look at Combine.globally
> <
> https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Combine.java#L124
> >.
> It returns a Globally, but that is because Globally has a useful public API
> surface, adding functions like asSingletonView. I believe similar reasoning
> applies to Count.
>
> However, for cases where the user will not further configure the return
> value, it makes sense to return the most public type we can.
>
> On Fri, Jan 27, 2017 at 6:39 AM, Jesse Anderson <[email protected]>
> wrote:
>
> > One con to making transform classes be private would be that it is a
> > breaking change. If anyone uses that class directly or extends that
> class,
> > we'd be breaking that.
> >
> > On Fri, Jan 27, 2017 at 9:37 AM Jesse Anderson <[email protected]>
> > wrote:
> >
> > > Continuing a discussion <https://github.com/apache/beam/pull/1830>
> Dan,
> > > Kenn, and I were having here since the bug is closed. They pointed out
> > > three things:
> > >
> > > - Where the private constructor gets placed in the class
> > > - Where the code samples of how to use the class get placed (in the
> > > Transform versus in the static method)
> > > - Are transform classes public or private
> > >
> > > I noted that those were inconsistent in the code. When I write a new
> > > transform, I use one of the already written transforms as the basis.
> > >
> > > Looking at Combine and Count:
> > >
> > > - The private constructor is at the top of the class
> > > - The code sample is in the Transform class
> > > - The transform class is marked as public
> > >
> > > I don't have a strong opinion on private constructor and transform
> being
> > > marked as private/public. I think we should standardize on placing code
> > > samples in the static helper methods. That's where people are looking
> > when
> > > using these methods.
> > >
> > > I think we need to do a general pass to make these consistent after we
> > > decide on how they should be done.
> > >
> > > Thanks,
> > >
> > > Jesse
> > >
> >
>
