Skip to content
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

Reorganize encoding.py so it doesn't rely on imports in __init__.py #234

Merged
merged 2 commits into from
Jun 15, 2018

Conversation

HarrisonGregg
Copy link
Contributor

Prerequisite for addressing #232.

Currently, when you import apitools.base.py.encoding, it triggers the imports in apitools/base/py/__init__.py. This includes importing extra_types.py, which makes patches to encoding, such as:

encoding.RegisterCustomMessageCodec(
    encoder=JsonProtoEncoder, decoder=_JsonToJsonValue)(JsonValue)

That means that if you remove the imports in __init__.py, then encoding behaves differently because extra_types isn't imported. This change reorganizes encoding.py, so that whenever it is imported, extra_types.py is also imported.

@HarrisonGregg HarrisonGregg force-pushed the reorganize-encoding branch 3 times, most recently from fa0d130 to 51b7e41 Compare June 14, 2018 19:59
@vilasj vilasj self-requested a review June 15, 2018 17:19
@vilasj
Copy link
Contributor

vilasj commented Jun 15, 2018

@HarrisonGregg HarrisonGregg force-pushed the reorganize-encoding branch 2 times, most recently from 94889db to a43a899 Compare June 15, 2018 17:47
Copy link
Contributor

@vilasj vilasj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified some questions about the changes offline including:

  • What happens if code is not moved into encoding_helper.py?

Turns out there is a cycle.

  • What happens if all is moved to encoding_helper.py?

Other code importing those encoding methods would break.

@HarrisonGregg HarrisonGregg force-pushed the reorganize-encoding branch 8 times, most recently from 2aeb767 to d020d7c Compare June 15, 2018 18:48
@coveralls
Copy link

coveralls commented Jun 15, 2018

Coverage Status

Coverage increased (+0.003%) to 91.686% when pulling 3845f73 on HarrisonGregg:reorganize-encoding into 48f6034 on google:master.

@vilasj vilasj merged commit deb0209 into google:master Jun 15, 2018
sixolet pushed a commit to sixolet/apitools that referenced this pull request Aug 7, 2018
…oogle#234)

* Move functionality in encoding.py to encoding_helper.py

* Add encoding.py file that imports encoding_helper and extra_types
sixolet pushed a commit to sixolet/apitools that referenced this pull request Aug 7, 2018
…oogle#234)

* Move functionality in encoding.py to encoding_helper.py

* Add encoding.py file that imports encoding_helper and extra_types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants