Skip to content

Fix __all__ in sagemaker.rl #553

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

Closed
wants to merge 1 commit into from
Closed

Fix __all__ in sagemaker.rl #553

wants to merge 1 commit into from

Conversation

tyrion
Copy link

@tyrion tyrion commented Dec 13, 2018

__all__ should be a list of names (str), not objects.
Moreover it

should be placed after the module docstring but before any import statements except from __future__ imports (PEP8)

I confirm that my contribution is made under the terms of the Apache 2.0 license.

__all__ should be a list of names (str), not objects.
Moreover it "should be placed after the module docstring but before any import
statements except from __future__ imports" (see PEP8)
@codecov-io
Copy link

codecov-io commented Dec 13, 2018

Codecov Report

Merging #553 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #553   +/-   ##
=======================================
  Coverage   92.77%   92.77%           
=======================================
  Files          71       71           
  Lines        5359     5359           
=======================================
  Hits         4972     4972           
  Misses        387      387
Impacted Files Coverage Δ
src/sagemaker/rl/__init__.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4bac185...5103239. Read the comment docs.

nadiaya
nadiaya previously approved these changes Dec 13, 2018
@nadiaya nadiaya dismissed their stale review December 13, 2018 20:12

Not consistent with other estimators

@tyrion
Copy link
Author

tyrion commented Dec 14, 2018

I am sorry, I did not notice this issue was present also in other estimators.
Let me know if you want me to open an issue regarding it, or to attempt a more generic fix in this pull request.

@laurenyu
Copy link
Contributor

@tyrion sorry for the delay in response. If you're up for fixing this in the rest of the estimators, feel free to do so in this PR. Otherwise, if you open an issue about it, one of us can probably take care of it

@laurenyu
Copy link
Contributor

sorry for the back and forth. talked offline with @nadiaya - I'm going to close this PR in favor of creating a new one that removes __all__ for consistency with the majority of our init files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants