Skip to content

Conversation

@dhermes
Copy link
Contributor

@dhermes dhermes commented Nov 30, 2015

Pylint 1.5 brought with it much more than

    pylint.extensions.check_docs

The changes for this PR are

  • checking import order
  • checking self._foo is set in __init__
  • complaining about methods that don't use self
  • complaining about not calling super().__init__ (for
    the virtual base class we used)
  • complaining about not (self == other) in __ne__,
    suggested (self != other) instead
  • complaining about unit tests checking None result from methods
    which are known (by static analysis?) to return None
  • complaining about not over-riding virtual method in storage
    _PropertyMixin
  • aggressively checking redefined-variable-type when variables
    can be optionally float /int / Entity / Key / etc.
  • checking that GCloudError constructor is called correctly
    in unit tests
  • checking that subclass method overrides use the same arguments
    (this occurred in BigQuery test_job with _makeResource)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 30, 2015
@dhermes
Copy link
Contributor Author

dhermes commented Nov 30, 2015

Pylint 1.5 was released yesterday (Nov. 29, 2015). This broke our latest build:
https://travis-ci.org/GoogleCloudPlatform/gcloud-python/builds/93984109

Pylint 1.4.4 was released on June 30, 2015 and the maintainers mentioned

My plan is to have 1.5 released on 15-17 july.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@dhermes
Copy link
Contributor Author

dhermes commented Nov 30, 2015

@tseaver I rebased this on top of #1249. PTAL

This comment was marked as spam.

This comment was marked as spam.

@dhermes dhermes force-pushed the update-for-new-pylint branch from 1944ab5 to 3073d32 Compare November 30, 2015 20:43
@dhermes
Copy link
Contributor Author

dhermes commented Nov 30, 2015

@tseaver I addressed the __init__ business for _access_grants and _schema and folded it into the original commit.

@tseaver
Copy link
Contributor

tseaver commented Nov 30, 2015

Seems like we're good except the issue of returning False vs. NotImplemented from __eq__ (entity and key).

Pylint 1.5 brought with it much more than
    pylint.extensions.check_docs
The changes for this PR are
- checking import order
- checking self._foo is set in __init__
- complaining about methods that don't use self
- complaining about not calling super().__init__ (for
  the virtual base class we used)
- complaining about not (self == other) in __ne__,
  suggested (self != other) instead
- complaining about unit tests checking None result from methods
  which are known (by static analysis?) to return None
- complaining about not over-riding virtual method in storage
  _PropertyMixin
- aggressively checking `redefined-variable-type` when variables
  can be optionally float/int/Entity/Key/etc.
- checking that GCloudError constructor is called correctly
  in unit tests
- checking that subclass method overrides use the same arguments
  (this occurred in BigQuery test_job with _makeResource)
@dhermes dhermes force-pushed the update-for-new-pylint branch from 3073d32 to 0c88da4 Compare November 30, 2015 21:13
@dhermes
Copy link
Contributor Author

dhermes commented Nov 30, 2015

@tseaver PTAL. Folded in the assignment-from-no-return and ditched NotImplemented as return value from __eq__.

Only open question left is local vs. global disabling of wrong-import-position. We can open an issue and discuss if you don't want to block on this PR?

@tseaver
Copy link
Contributor

tseaver commented Nov 30, 2015

We can let the current local disabling go, I think: I don't know what it would take to get a "conforming" version in place (one which wouldn't require the local disable). So, LGTM to merge.

dhermes added a commit that referenced this pull request Nov 30, 2015
@dhermes dhermes merged commit 0f8bd8e into googleapis:master Nov 30, 2015
@dhermes dhermes deleted the update-for-new-pylint branch November 30, 2015 21:58
@dhermes
Copy link
Contributor Author

dhermes commented Nov 30, 2015

It's impossible to get a conforming version in place. You've inspired me to file an issue with the Pylint folks.

The issue was that using

try:
    import foo
except ImportError:
    ...

import bar

makes the bar import invalid no matter what, since the try/except is considered code (i.e. imports should be over by then).

But for some pairs of foo / bar (like if one is in the stdlib and the other is local to our library), swapping them

import bar

try:
    import foo
except ImportError:
    ...

makes Pylint angry that the import of bar comes before the import of foo.

@dhermes
Copy link
Contributor Author

dhermes commented Nov 30, 2015

@tseaver Maybe our rcfile is out of date? I'm going to check it out.

@dhermes
Copy link
Contributor Author

dhermes commented Nov 30, 2015

With the current order (and no ignores):

diff --git a/gcloud/_helpers.py b/gcloud/_helpers.py
index b784042..2d44b99 100644
--- a/gcloud/_helpers.py
+++ b/gcloud/_helpers.py
@@ -27,12 +27,10 @@ from six.moves.http_client import HTTPConnection  # pylint: disable=F0401

 from gcloud.environment_vars import PROJECT

-# pylint: disable=wrong-import-position
 try:
     from google.appengine.api import app_identity
 except ImportError:
     app_identity = None
-# pylint: enable=wrong-import-position


 _NOW = datetime.datetime.utcnow  # To be replaced by tests.
diff --git a/gcloud/credentials.py b/gcloud/credentials.py
index bfe8926..cd61b7f 100644
--- a/gcloud/credentials.py
+++ b/gcloud/credentials.py
@@ -32,7 +32,6 @@ except ImportError:
     class _GAECreds(object):
         """Dummy class if not in App Engine environment."""

-# pylint: disable=wrong-import-position
 try:
     from google.appengine.api import app_identity
 except ImportError:
@@ -41,7 +40,6 @@ except ImportError:
 from gcloud._helpers import UTC
 from gcloud._helpers import _NOW
 from gcloud._helpers import _microseconds_from_datetime
-# pylint: enable=wrong-import-position


 def get_credentials():

we get the following errors:

C: 36, 4: Import "from google.appengine.api import app_identity" should be placed at the top of the module (wrong-import-position)
C: 40, 0: Import "from gcloud._helpers import UTC" should be placed at the top of the module (wrong-import-position)
C: 41, 0: Import "from gcloud._helpers import _NOW" should be placed at the top of the module (wrong-import-position)
C: 42, 0: Import "from gcloud._helpers import _microseconds_from_datetime" should be placed at the top of the module (wrong-import-position)

Trying to swap the order:

diff --git a/gcloud/_helpers.py b/gcloud/_helpers.py
index b784042..2d44b99 100644
--- a/gcloud/_helpers.py
+++ b/gcloud/_helpers.py
@@ -27,12 +27,10 @@ from six.moves.http_client import HTTPConnection  # pylint: disable=F0401

 from gcloud.environment_vars import PROJECT

-# pylint: disable=wrong-import-position
 try:
     from google.appengine.api import app_identity
 except ImportError:
     app_identity = None
-# pylint: enable=wrong-import-position


 _NOW = datetime.datetime.utcnow  # To be replaced by tests.
diff --git a/gcloud/credentials.py b/gcloud/credentials.py
index bfe8926..11d7227 100644
--- a/gcloud/credentials.py
+++ b/gcloud/credentials.py
@@ -26,23 +26,22 @@ from oauth2client import client
 from oauth2client.client import _get_application_default_credential_from_file
 from oauth2client import crypt
 from oauth2client import service_account
+
+from gcloud._helpers import UTC
+from gcloud._helpers import _NOW
+from gcloud._helpers import _microseconds_from_datetime
+
 try:
     from oauth2client.appengine import AppAssertionCredentials as _GAECreds
 except ImportError:
     class _GAECreds(object):
         """Dummy class if not in App Engine environment."""

-# pylint: disable=wrong-import-position
 try:
     from google.appengine.api import app_identity
 except ImportError:
     app_identity = None

-from gcloud._helpers import UTC
-from gcloud._helpers import _NOW
-from gcloud._helpers import _microseconds_from_datetime
-# pylint: enable=wrong-import-position
-

 def get_credentials():
     """Gets credentials implicitly from the current environment.

we get the following errors

C: 41, 4: Import "from google.appengine.api import app_identity" should be placed at the top of the module (wrong-import-position)
C: 35, 4: external import "from oauth2client.appengine import AppAssertionCredentials as _GAECreds" comes before "from gcloud._helpers import UTC" (wrong-import-order)

@dhermes
Copy link
Contributor Author

dhermes commented Nov 30, 2015

Filed: https://bitbucket.org/logilab/pylint/issues/714/

It seems if our except blocks are more vanilla (i.e. don't define a class right then and there) then it is still considered an import.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@dhermes
Copy link
Contributor Author

dhermes commented Dec 1, 2015

parthea pushed a commit that referenced this pull request Nov 24, 2025
* Add Error Reporting Code

* Updating doc links [(#324)](GoogleCloudPlatform/python-docs-samples#324)

* Commenting noxfile, renaming a test file.

Change-Id: Ice25caa7c035c24a585a66575dda69e170862df8

* Update requirements. [(#436)](GoogleCloudPlatform/python-docs-samples#436)

* Fix import order lint errors

Change-Id: Ieaf7237fc6f925daec46a07d2e81a452b841198a

* bump

Change-Id: I02e7767d13ba267ee9fc72c5b68a57013bb8b8d3

* Auto-update dependencies. [(#537)](GoogleCloudPlatform/python-docs-samples#537)

* Update samples to support latest Google Cloud Python [(#656)](GoogleCloudPlatform/python-docs-samples#656)

* Switch Error Reporting to Google Cloud Python [(#663)](GoogleCloudPlatform/python-docs-samples#663)

* Switch Error Reporting to Google Cloud Python

* Switch Error Reporting to Google Cloud Python

* Auto-update dependencies. [(#715)](GoogleCloudPlatform/python-docs-samples#715)

* Auto-update dependencies. [(#914)](GoogleCloudPlatform/python-docs-samples#914)

* Auto-update dependencies.

* xfail the error reporting test

* Fix lint

* Re-generate all readmes

* Auto-update dependencies. [(#922)](GoogleCloudPlatform/python-docs-samples#922)

* Auto-update dependencies.

* Fix pubsub iam samples

* Fix README rst links [(#962)](GoogleCloudPlatform/python-docs-samples#962)

* Fix README rst links

* Update all READMEs

* Auto-update dependencies. [(#1004)](GoogleCloudPlatform/python-docs-samples#1004)

* Auto-update dependencies.

* Fix natural language samples

* Fix pubsub iam samples

* Fix language samples

* Fix bigquery samples

* Auto-update dependencies. [(#1011)](GoogleCloudPlatform/python-docs-samples#1011)

* Auto-update dependencies. [(#1055)](GoogleCloudPlatform/python-docs-samples#1055)

* Auto-update dependencies.

* Explicitly use latest bigtable client

Change-Id: Id71e9e768f020730e4ca9514a0d7ebaa794e7d9e

* Revert language update for now

Change-Id: I8867f154e9a5aae00d0047c9caf880e5e8f50c53

* Remove pdb. smh

Change-Id: I5ff905fadc026eebbcd45512d4e76e003e3b2b43

* Auto-update dependencies. [(#1093)](GoogleCloudPlatform/python-docs-samples#1093)

* Auto-update dependencies.

* Fix storage notification poll sample

Change-Id: I6afbc79d15e050531555e4c8e51066996717a0f3

* Fix spanner samples

Change-Id: I40069222c60d57e8f3d3878167591af9130895cb

* Drop coverage because it's not useful

Change-Id: Iae399a7083d7866c3c7b9162d0de244fbff8b522

* Try again to fix flaky logging test

Change-Id: I6225c074701970c17c426677ef1935bb6d7e36b4

* Update all generated readme auth instructions [(#1121)](GoogleCloudPlatform/python-docs-samples#1121)

Change-Id: I03b5eaef8b17ac3dc3c0339fd2c7447bd3e11bd2

* Added Link to Python Setup Guide [(#1158)](GoogleCloudPlatform/python-docs-samples#1158)

* Update Readme.rst to add Python setup guide

As requested in b/64770713.

This sample is linked in documentation https://cloud.google.com/bigtable/docs/scaling, and it would make more sense to update the guide here than in the documentation.

* Update README.rst

* Update README.rst

* Update README.rst

* Update README.rst

* Update README.rst

* Update install_deps.tmpl.rst

* Updated readmegen scripts and re-generated related README files

* Fixed the lint error

* Auto-update dependencies. [(#1186)](GoogleCloudPlatform/python-docs-samples#1186)

* Add error reporting sample for manual reporting

* Readd fluentd samples

* rename dir

* Auto-update dependencies. [(#1205)](GoogleCloudPlatform/python-docs-samples#1205)

* Auto-update dependencies. [(#1215)](GoogleCloudPlatform/python-docs-samples#1215)

* Auto-update dependencies. [(#1245)](GoogleCloudPlatform/python-docs-samples#1245)

* Auto-update dependencies. [(#1248)](GoogleCloudPlatform/python-docs-samples#1248)

* Added "Open in Cloud Shell" buttons to README files [(#1254)](GoogleCloudPlatform/python-docs-samples#1254)

* Auto-update dependencies. [(#1276)](GoogleCloudPlatform/python-docs-samples#1276)

* Auto-update dependencies. [(#1282)](GoogleCloudPlatform/python-docs-samples#1282)

* Auto-update dependencies.

* Fix storage acl sample

Change-Id: I413bea899fdde4c4859e4070a9da25845b81f7cf

* Auto-update dependencies. [(#1317)](GoogleCloudPlatform/python-docs-samples#1317)

* Auto-update dependencies. [(#1320)](GoogleCloudPlatform/python-docs-samples#1320)

* Auto-update dependencies. [(#1359)](GoogleCloudPlatform/python-docs-samples#1359)

* Auto-update dependencies. [(#1377)](GoogleCloudPlatform/python-docs-samples#1377)

* Auto-update dependencies.

* Update requirements.txt

* Regenerate the README files and fix the Open in Cloud Shell link for some samples [(#1441)](GoogleCloudPlatform/python-docs-samples#1441)

* Update READMEs to fix numbering and add git clone [(#1464)](GoogleCloudPlatform/python-docs-samples#1464)

* Auto-update dependencies. [(#1658)](GoogleCloudPlatform/python-docs-samples#1658)

* Auto-update dependencies.

* Rollback appengine/standard/bigquery/.

* Rollback appengine/standard/iap/.

* Rollback bigtable/metricscaler.

* Rolledback appengine/flexible/datastore.

* Rollback dataproc/

* Rollback jobs/api_client

* Rollback vision/cloud-client.

* Rollback functions/ocr/app.

* Rollback iot/api-client/end_to_end_example.

* Rollback storage/cloud-client.

* Rollback kms/api-client.

* Rollback dlp/

* Rollback bigquery/cloud-client.

* Rollback iot/api-client/manager.

* Rollback appengine/flexible/cloudsql_postgresql.

* Put in new region code error_reporting_quickstart [(#1842)](GoogleCloudPlatform/python-docs-samples#1842)

* New region tags [(#1843)](GoogleCloudPlatform/python-docs-samples#1843)

* Auto-update dependencies. [(#1980)](GoogleCloudPlatform/python-docs-samples#1980)

* Auto-update dependencies.

* Update requirements.txt

* Update requirements.txt

* Adds updates including compute [(#2436)](GoogleCloudPlatform/python-docs-samples#2436)

* Adds updates including compute

* Python 2 compat pytest

* Fixing weird \r\n issue from GH merge

* Put asset tests back in

* Re-add pod operator test

* Hack parameter for k8s pod operator

* Auto-update dependencies. [(#2005)](GoogleCloudPlatform/python-docs-samples#2005)

* Auto-update dependencies.

* Revert update of appengine/flexible/datastore.

* revert update of appengine/flexible/scipy

* revert update of bigquery/bqml

* revert update of bigquery/cloud-client

* revert update of bigquery/datalab-migration

* revert update of bigtable/quickstart

* revert update of compute/api

* revert update of container_registry/container_analysis

* revert update of dataflow/run_template

* revert update of datastore/cloud-ndb

* revert update of dialogflow/cloud-client

* revert update of dlp

* revert update of functions/imagemagick

* revert update of functions/ocr/app

* revert update of healthcare/api-client/fhir

* revert update of iam/api-client

* revert update of iot/api-client/gcs_file_to_device

* revert update of iot/api-client/mqtt_example

* revert update of language/automl

* revert update of run/image-processing

* revert update of vision/automl

* revert update testing/requirements.txt

* revert update of vision/cloud-client/detect

* revert update of vision/cloud-client/product_search

* revert update of jobs/v2/api_client

* revert update of jobs/v3/api_client

* revert update of opencensus

* revert update of translate/cloud-client

* revert update to speech/cloud-client

Co-authored-by: Kurtis Van Gent <[email protected]>
Co-authored-by: Doug Mahugh <[email protected]>

* chore(deps): update dependency fluent-logger to v0.9.5 [(#3004)](GoogleCloudPlatform/python-docs-samples#3004)

* chore(deps): update dependency fluent-logger to v0.9.6 [(#3080)](GoogleCloudPlatform/python-docs-samples#3080)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [fluent-logger](https://togithub.com/fluent/fluent-logger-python) | patch | `==0.9.5` -> `==0.9.6` |

---

### Release Notes

<details>
<summary>fluent/fluent-logger-python</summary>

### [`v0.9.6`](https://togithub.com/fluent/fluent-logger-python/compare/v0.9.5...v0.9.6)

[Compare Source](https://togithub.com/fluent/fluent-logger-python/compare/v0.9.5...v0.9.6)

</details>

---

### Renovate configuration

:date: **Schedule**: At any time (no schedule defined).

:vertical_traffic_light: **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

:recycle: **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

:no_bell: **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#GoogleCloudPlatform/python-docs-samples).

* Simplify noxfile setup. [(#2806)](GoogleCloudPlatform/python-docs-samples#2806)

* chore(deps): update dependency requests to v2.23.0

* Simplify noxfile and add version control.

* Configure appengine/standard to only test Python 2.7.

* Update Kokokro configs to match noxfile.

* Add requirements-test to each folder.

* Remove Py2 versions from everything execept appengine/standard.

* Remove conftest.py.

* Remove appengine/standard/conftest.py

* Remove 'no-sucess-flaky-report' from pytest.ini.

* Add GAE SDK back to appengine/standard tests.

* Fix typo.

* Roll pytest to python 2 version.

* Add a bunch of testing requirements.

* Remove typo.

* Add appengine lib directory back in.

* Add some additional requirements.

* Fix issue with flake8 args.

* Even more requirements.

* Readd appengine conftest.py.

* Add a few more requirements.

* Even more Appengine requirements.

* Add webtest for appengine/standard/mailgun.

* Add some additional requirements.

* Add workaround for issue with mailjet-rest.

* Add responses for appengine/standard/mailjet.

Co-authored-by: Renovate Bot <[email protected]>

* chore(deps): update dependency google-cloud-error-reporting to v0.34.0 [(#3782)](GoogleCloudPlatform/python-docs-samples#3782)

* chore(deps): update dependency pytest to v5.4.3 [(#4279)](GoogleCloudPlatform/python-docs-samples#4279)

* chore(deps): update dependency pytest to v5.4.3

* specify pytest for python 2 in appengine

Co-authored-by: Leah Cole <[email protected]>

* chore(deps): update dependency mock to v4 [(#4287)](GoogleCloudPlatform/python-docs-samples#4287)

* chore(deps): update dependency mock to v4

* specify mock version for appengine python 2

Co-authored-by: Leah Cole <[email protected]>

* Update dependency pytest to v6 [(#4390)](GoogleCloudPlatform/python-docs-samples#4390)

* chore: update templates

Co-authored-by: Bill Prin <[email protected]>
Co-authored-by: Jon Wayne Parrott <[email protected]>
Co-authored-by: DPE bot <[email protected]>
Co-authored-by: Bill Prin <[email protected]>
Co-authored-by: michaelawyu <[email protected]>
Co-authored-by: Andrew Gorcester <[email protected]>
Co-authored-by: Andrew Gorcester <[email protected]>
Co-authored-by: Frank Natividad <[email protected]>
Co-authored-by: Charles Engelke <[email protected]>
Co-authored-by: Gus Class <[email protected]>
Co-authored-by: Kurtis Van Gent <[email protected]>
Co-authored-by: Doug Mahugh <[email protected]>
Co-authored-by: WhiteSource Renovate <[email protected]>
Co-authored-by: Leah Cole <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement. testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants