New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BEAM-1345] Clearly delineate public api in apache_beam/coders. #3090
Conversation
can we remove the coders import from the root apache_beam/init.py then? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
"""For internal use only; no backwards-compatibility guarantees. | ||
|
||
Returns the CoderImpl backing this Coder. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please also mark as_cloud_object()
, get_estimated_size_and_observables()
and the *_runner_api()
functions in this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
"""For internal use only; no backwards-compatibility guarantees. | ||
|
||
Returns the CoderImpl backing this Coder. | ||
""" | ||
if not hasattr(self, '_impl'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also mark LengthPrefixCoder
as internal / experimental?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -21,7 +21,7 @@ | |||
from __future__ import absolute_import | |||
import logging | |||
|
|||
from apache_beam import coders | |||
from apache_beam.coders import coders |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the guidance now for usage of the Coder
class? Are users expected to do something like this?
class MyCoder(beam.coders.coders.Coder):
...
Did you also mean to change coders/__init__.py
, or is the simpler form (beam.coders.Coder
) still permitted / recommended in user code? (CC: @sb2nov)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coders/__init__.py
changed because of the __all__
in coders/coders.py
This is only needed because it uses the no-public ToString Coder. beam.coders.Coder
is still recommended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Removing coders from |
PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM.
@@ -21,7 +21,7 @@ | |||
from __future__ import absolute_import | |||
import logging | |||
|
|||
from apache_beam import coders | |||
from apache_beam.coders import coders |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Will merge as soon as tests finish (green). |
@aaltay for cherry-pick |
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull request
mvn clean verify
.<Jira issue #>
in the title with the actual Jira issuenumber, if there is one.
Individual Contributor License Agreement.