Skip to content

change: enable wrong-import-position pylint check #907

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jul 9, 2019
Merged

change: enable wrong-import-position pylint check #907

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bc57c5e
change: enable wrong-import-position pylint check
knakad Jul 5, 2019
08c3722
Merge branch 'master' into 20190705-lint-tighten-import-order
knakad Jul 5, 2019
20197b3
change: updating import pattern for sagemaker.tensorflow
knakad Jul 8, 2019
414a468
Merge branch 'master' into 20190705-lint-tighten-import-order
knakad Jul 8, 2019
9249318
Merge branch 'master' into 20190705-lint-tighten-import-order
knakad Jul 9, 2019
ff57e34
fix: fixing integration tests
knakad Jul 9, 2019
4a19235
change: reformatting
knakad Jul 9, 2019
fded477
Merge branch 'master' into 20190705-lint-tighten-import-order
knakad Jul 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .pylintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@ disable=
simplifiable-if-expression, # TODO: Simplify expressions
too-many-public-methods, # TODO: Resolve
ungrouped-imports, # TODO: Group imports
wrong-import-position, # TODO: Correct import positions
consider-using-ternary, # TODO: Consider ternary expressions
chained-comparison, # TODO: Simplify chained comparison between operands
simplifiable-if-statement, # TODO: Simplify ifs
Expand Down
3 changes: 2 additions & 1 deletion src/sagemaker/amazon/record_pb2.py
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 9 additions & 2 deletions src/sagemaker/tensorflow/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,12 @@
# classes for tensorflow serving. Currently tensorflow_serving_api can only be pip-installed for python 2.
sys.path.append(os.path.dirname(__file__))

from sagemaker.tensorflow.estimator import TensorFlow # noqa: E402, F401
from sagemaker.tensorflow.model import TensorFlowModel, TensorFlowPredictor # noqa: E402, F401
from sagemaker.tensorflow.estimator import ( # noqa: E402,F401 # pylint: disable=wrong-import-position
TensorFlow,
)
from sagemaker.tensorflow.model import ( # noqa: E402,F401 # pylint: disable=wrong-import-position
TensorFlowModel,
)
from sagemaker.tensorflow.model import ( # noqa: E402,F401 # pylint: disable=wrong-import-position
TensorFlowPredictor,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not:

from sagemaker.tensorflow import model, estimator

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'll work as well, but shouldn't we aim to be more restrictive in our imports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, per the google python style guide, we should aim to import modules, not classes. Good callout!
Also per the google style guide, I went ahead and made the change in two lines.