[BEAM-1340] Add __all__ tags to modules in package apache_beam/transforms#3075
[BEAM-1340] Add __all__ tags to modules in package apache_beam/transforms#3075charlesccychen wants to merge 3 commits intoapache:masterfrom
Conversation
|
R: @robertwb |
|
|
||
|
|
||
| __all__ = [ | ||
| 'DoFnContext', |
There was a problem hiding this comment.
DoFnContext should not be public.
| 'DoFnContext', | ||
| 'DoFnProcessContext', | ||
| 'DoFn', | ||
| 'CallableWrapperDoFn', |
There was a problem hiding this comment.
The CallableWrapper* classes shouldn't be public.
| 'CombineGlobally', | ||
| 'CombinePerKey', | ||
| 'CombineValues', | ||
| 'CombineValuesDoFn', |
There was a problem hiding this comment.
CombineValuesDoFn should not be public.
| 'CombineValues', | ||
| 'CombineValuesDoFn', | ||
| 'GroupByKey', | ||
| 'ReifyWindows', |
There was a problem hiding this comment.
ReifyWindows, GroupAlsoByWIndows, and GroupByKeyOnly should not be public.
| 'GroupAlsoByWindow', | ||
| 'GroupByKeyOnly', | ||
| 'Partition', | ||
| 'ApplyPartitionFnFn', |
There was a problem hiding this comment.
ApplyPartitonFn should not be public.
|
|
||
|
|
||
| __all__ = [ | ||
| 'GetPValues', |
There was a problem hiding this comment.
The only exported things from here should be PTransform and possibly label_from_callable. Maybe ptransform_fn, but mark it clearly as experimental (CloudML had some closure issues that were never fully resolved). The rest are internal implementation details.
|
|
||
|
|
||
| __all__ = [ | ||
| 'default_window_mapping_fn', |
There was a problem hiding this comment.
Don't expose anything from this module (yet).
|
|
||
|
|
||
| __all__ = [ | ||
| 'TimeDomain', |
There was a problem hiding this comment.
Only TimeDomain should be exported from this module.
| 'AfterAll', | ||
| 'AfterEach', | ||
| 'OrFinally', | ||
| 'TriggerContext', |
There was a problem hiding this comment.
TriggerContext and below are an implementation details.
| 'FixedWindows', | ||
| 'SlidingWindows', | ||
| 'Sessions', | ||
| ] |
There was a problem hiding this comment.
@aaltay Was the intent to export WindowedValue from here rather than from utils?
There was a problem hiding this comment.
Or should we keep it Experimental for now (it's not an exposed concept in Java).
There was a problem hiding this comment.
I think we should move WindowedValue out of util to here and also mark it as experimental. (cc: @sb2nov)
|
Thanks, PTAL. |
robertwb
left a comment
robertwb
left a comment
There was a problem hiding this comment.
Just one comment, otherwise LGTM.
| def ptransform_fn(fn): | ||
| """A decorator for a function-based PTransform. | ||
|
|
||
| For internal use only; no backwards-compatibility guarantees. |
There was a problem hiding this comment.
Not for internal use, just note that it's expiremental (and, yes, not backwards compatible).
| from apache_beam.transforms import window | ||
|
|
||
|
|
||
| __all__ = [] |
There was a problem hiding this comment.
This may cause lint errors. Remove.
|
[ignore] |
charlesccychen
changed the title
|
Thanks, PTAL. |
|
LGTM |
|
There were a couple of trivial test failures (e.g. due to no beam.GroupByKeyOnly). Fixing as part of the merge. |
4 participants
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull requestmvn clean verify.<Jira issue #>in the title with the actual Jira issuenumber, if there is one.
Individual Contributor License Agreement.