Skip to content

Commit

Permalink
Feature/import warnings (#3648)
Browse files Browse the repository at this point in the history
* update class names

* docs

* tests

* fixme

* change to convention

* only allow imports from transport

* spacing

* naming

* better naming

* typo in tests

* update linter names

* fix test (please)

* better class name
  • Loading branch information
Steven Jin authored Aug 2, 2022
1 parent a1b988d commit a3c131e
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 5 deletions.
3 changes: 2 additions & 1 deletion tools/pylint-extensions/pylint-guidelines-checker/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,4 +73,5 @@ In the case of a false positive, use the disable command to remove the pylint er
| naming-mismatch | Do not alias models imported from the generated code. | # pylint:disable=naming-mismatch | [link](https://github.com/Azure/autorest/blob/main/docs/generate/built-in-directives.md) |
| client-accepts-api-version-keyword | Ensure that the client constructor accepts a keyword-only api_version argument. | # pylint:disable=client-accepts-api-version-keyword | [link](https://azure.github.io/azure-sdk/python_design.html#specifying-the-service-version) |
| enum-must-be-uppercase | The enum name must be all uppercase. | # pylint:disable=enum-must-be-uppercase | [link](https://azure.github.io/azure-sdk/python_design.html#enumerations) |
| enum-must-inherit-case-insensitive-enum-meta | The enum should inherit from CaseInsensitiveEnumMeta. | # pylint:disable=enum-must-inherit-case-insensitive-enum-meta | [link](https://azure.github.io/azure-sdk/python_implementation.html#extensible-enumerations) |
| enum-must-inherit-case-insensitive-enum-meta | The enum should inherit from CaseInsensitiveEnumMeta. | # pylint:disable=enum-must-inherit-case-insensitive-enum-meta | [link](https://azure.github.io/azure-sdk/python_implementation.html#extensible-enumerations) |
| networking-import-outside-azure-core-transport | This import is only allowed in azure.core.pipeline.transport. | # pylint:disable=networking-import-outside-azure-core-transport | [link](https://github.com/Azure/azure-sdk-for-python/issues/24989) |
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
# ------------------------------------

"""
Pylint custom checkers for SDK guidelines: C4717 - C4748
Pylint custom checkers for SDK guidelines: C4717 - C4749
"""

import logging
Expand Down Expand Up @@ -1916,7 +1916,45 @@ def visit_module(self, node):

except Exception:
logger.debug("Pylint custom checker failed to check if model is aliased.")
pass

class NonCoreNetworkImport(BaseChecker):
"""There are certain imports that should only occur in the core package.
For example, instead of using `requests` to make requests, clients should
take a `azure.core.pipeline.Pipeline` as input to make requests.
"""
name = "networking-import-outside-azure-core-transport"
priority = -1
msgs = {
"C4749": (
"This import is not allowed here. Consider using an abstract"
" alternative from azure.core.pipeline.transport.",
"networking-import-outside-azure-core-transport",
"This import is only allowed in azure.core.pipeline.transport.",
),
}
BLOCKED_MODULES = ["aiohttp", "requests", "trio"]
AZURE_CORE_TRANSPORT_NAME = "azure.core.pipeline.transport"

def visit_import(self, node):
"""Check that we dont have blocked imports."""
if node.root().name.startswith(self.AZURE_CORE_TRANSPORT_NAME):
return
for import_, _ in node.names:
self._check_import(import_, node)

def visit_importfrom(self, node):
"""Check that we aren't import from a blocked package."""
if node.root().name.startswith(self.AZURE_CORE_TRANSPORT_NAME):
return
self._check_import(node.modname, node)

def _check_import(self, name, node):
"""Check if an import is blocked."""
for blocked in self.BLOCKED_MODULES:
if name.startswith(blocked):
self.add_message(
msgid=f"networking-import-outside-azure-core-transport", node=node, confidence=None
)


# if a linter is registered in this function then it will be checked with pylint
Expand All @@ -1939,6 +1977,7 @@ def register(linter):
linter.register_checker(CheckNamingMismatchGeneratedCode(linter))
linter.register_checker(CheckAPIVersion(linter))
linter.register_checker(CheckEnum(linter))
linter.register_checker(NonCoreNetworkImport(linter))
linter.register_checker(ClientListMethodsUseCorePaging(linter))


Expand All @@ -1953,5 +1992,3 @@ def register(linter):
# linter.register_checker(ClientDocstringUsesLiteralIncludeForCodeExample(linter))
# linter.register_checker(ClientLROMethodsUseCorePolling(linter))
# linter.register_checker(ClientLROMethodsUseCorrectNaming(linter))


Original file line number Diff line number Diff line change
Expand Up @@ -3017,3 +3017,59 @@ def test_guidelines_link_active(self):
request = client.get(url)
response = client._pipeline.run(request)
assert response.http_response.status_code == 200


class TestCheckNonCoreNetworkImport(pylint.testutils.CheckerTestCase):
"""Test that we are blocking disallowed imports and allowing allowed imports."""
CHECKER_CLASS = checker.NonCoreNetworkImport

def test_disallowed_imports(self):
"""Check that illegal imports raise warnings"""
# Blocked import ouside of core.
import_node = astroid.extract_node("import requests")
with self.assertAddsMessages(
pylint.testutils.MessageTest(
msg_id="networking-import-outside-azure-core-transport",
line=1,
node=import_node,
col_offset=0,
)
):
self.checker.visit_import(import_node)

# blocked import from outside of core.
importfrom_node = astroid.extract_node("from aiohttp import get")
with self.assertAddsMessages(
pylint.testutils.MessageTest(
msg_id="networking-import-outside-azure-core-transport",
line=1,
node=importfrom_node,
col_offset=0,
)
):
self.checker.visit_importfrom(importfrom_node)


def test_allowed_imports(self):
"""Check that allowed imports don't raise warnings."""
# import not in the blocked list.
import_node = astroid.extract_node("import math")
with self.assertNoMessages():
self.checker.visit_import(import_node)

# from import not in the blocked list.
importfrom_node = astroid.extract_node("from azure.core.pipeline import Pipeline")
with self.assertNoMessages():
self.checker.visit_importfrom(importfrom_node)

# blocked import, but in core.
import_node = astroid.extract_node("import requests")
import_node.root().name = "azure.core.pipeline.transport"
with self.assertNoMessages():
self.checker.visit_import(import_node)

# blocked from import, but in core.
importfrom_node = astroid.extract_node("from requests.exceptions import HttpException")
importfrom_node.root().name = "azure.core.pipeline.transport._private_module"
with self.assertNoMessages():
self.checker.visit_importfrom(importfrom_node)

0 comments on commit a3c131e

Please sign in to comment.