Skip to content

Commit

Permalink
Merge branch 'working-main' into exception-logging
Browse files Browse the repository at this point in the history
  • Loading branch information
@JessicaBell00
JessicaBell00 authored Oct 6, 2024
2 parents 7e7256b + 18ea2fa commit e2dcbb9
Show file tree
Hide file tree
Showing 8 changed files with 843 additions and 325 deletions.
8 changes: 6 additions & 2 deletions tools/pylint-extensions/azure-pylint-guidelines-checker/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,11 @@ In the case of a false positive, use the disable command to remove the pylint er
| docstring-keyword-should-match-keyword-only | Docstring keyword arguments and keyword-only method arguments should match. | pylint:disable=docstring-keyword-should-match-keyword-only | [link](https://azure.github.io/azure-sdk/python_documentation.html#docstrings) |
| docstring-type-do-not-use-class | Docstring type is formatted incorrectly. Do not use `:class` in docstring type. | pylint:disable=docstring-type-do-not-use-class | [link](https://sphinx-rtd-tutorial.readthedocs.io/en/latest/docstrings.html) |
| no-typing-import-in-type-check | Do not import typing under TYPE_CHECKING. | pylint:disable=no-typing-import-in-type-check | No Link. |
| invalid-use-of-overload | Do not mix async and synchronous overloads | pylint:disable=invalid-use-of-overload | No Link. |
| do-not-log-raised-errors | Do not log errors at `error` or `warning` level when error is raised in an exception block. | pylint:disable=do-not-log-raised-errors | No Link. |
| do-not-use-legacy-typing | Do not use legacy (<Python 3.8) type hinting comments | pylint:disable=do-not-use-legacy-typing | No Link.
| do-not-use-legacy-typing | Do not use legacy (<Python 3.8) type hinting comments | pylint:disable=do-not-use-legacy-typing | No Link. |
| do-not-import-asyncio | Do not import asyncio directly. | pylint:disable=do-not-import-asyncio | No Link. |
| do-not-log-exceptions | Do not log exceptions in levels other than debug, otherwise it can reveal sensitive information | pylint:disable=do-not-log-exceptions | [link](https://azure.github.io/azure-sdk/python_implementation.html#python-logging-sensitive-info) |
| do-not-log-exceptions | Do not log exceptions in levels other than debug, otherwise it can reveal sensitive information | pylint:disable=do-not-log-exceptions | [link](https://azure.github.io/azure-sdk/python_implementation.html#python-logging-sensitive-info) | | | |
| unapproved-client-method-name-prefix | Clients should use preferred verbs for method names | pylint:disable=unapproved-client-method-name-prefix | [link](https://azure.github.io/azure-sdk/python_design.html#naming) |
| do-not-hardcode-connection-verify | Do not hardcode a boolean value to connection_verify | pylint:disable=do-not-hardcode-connection-verify | No LInk. |
| TODO | Address Commented out Pylint Custom Plugin Checkers #3228 | | |
289 changes: 221 additions & 68 deletions tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,79 +161,93 @@ class ClientHasApprovedMethodNamePrefix(BaseChecker):
" https://azure.github.io/azure-sdk/python_design.html#service-operations",
"unapproved-client-method-name-prefix",
"All clients should use the preferred verbs for method names.",
)
}
options = (
(
"ignore-unapproved-client-method-name-prefix",
{
"default": False,
"type": "yn",
"metavar": "<y_or_n>",
"help": "Allow clients to not use preferred method name prefixes",
},
),
)
}

ignore_clients = [
ignore_clients = [
"PipelineClient",
"AsyncPipelineClient",
"ARMPipelineClient",
"AsyncARMPipelineClient",
]

approved_prefixes = [
"get",
"list",
"create",
"upsert",
"set",
"update",
"replace",
"append",
"add",
"delete",
"remove",
"begin",
"upload",
"download", # standard verbs
"close", # very common verb
"cancel",
"clear",
"subscribe",
"send",
"query", # common verbs
"analyze",
"train",
"detect", # future proofing
"from", # special case
]

ignored_decorators = [
"property",
]

def __init__(self, linter=None):
super(ClientHasApprovedMethodNamePrefix, self).__init__(linter)
self.process_class = None
self.namespace = None

def _check_decorators(self, node):
if not node.decorators:
return False
for decorator in node.decorators.nodes:
if isinstance(decorator, astroid.nodes.Name) and decorator.name in self.ignored_decorators:
return True
return False

def visit_module(self, node):
self.namespace = node.name

def visit_classdef(self, node):
"""Visits every class in file and checks if it is a client. If it is a client, checks
that approved method name prefixes are present.
if all((
node.name.endswith("Client"),
node.name not in self.ignore_clients,
not node.name.startswith("_"),
not '._' in self.namespace,
)):
self.process_class = node

:param node: class node
:type node: ast.ClassDef
:return: None
"""
try:
if node.name.endswith("Client") and node.name not in self.ignore_clients:
client_methods = [
child for child in node.get_children() if child.is_function
]

approved_prefixes = [
"get",
"list",
"create",
"upsert",
"set",
"update",
"replace",
"append",
"add",
"delete",
"remove",
"begin",
]
for idx, method in enumerate(client_methods):
if (
method.name.startswith("__")
or "_exists" in method.name
or method.name.startswith("_")
or method.name.startswith("from")
):
continue
prefix = method.name.split("_")[0]
if prefix.lower() not in approved_prefixes:
self.add_message(
msgid="unapproved-client-method-name-prefix",
node=client_methods[idx],
confidence=None,
)
except AttributeError:
logger.debug(
"Pylint custom checker failed to check if client has approved method name prefix."
def visit_functiondef(self, node):
if any((
self.process_class is None, # not in a client class
node.name.startswith("_"), # private method
node.name.endswith("_exists"), # special case
self._check_decorators(node), # property
node.parent != self.process_class, # nested method
)):
return

# check for approved prefix
parts = node.name.split("_")
if parts[0].lower() not in self.approved_prefixes:
self.add_message(
msgid="unapproved-client-method-name-prefix",
node=node,
confidence=None,
)
pass

def leave_classdef(self, node):
self.process_class = None

class ClientMethodsUseKwargsWithMultipleParameters(BaseChecker):
name = "client-method-multiple-parameters"
Expand Down Expand Up @@ -2805,6 +2819,75 @@ def visit_import(self, node):
pass


class InvalidUseOfOverload(BaseChecker):
"""Rule to check that use of the @overload decorator matches the async/sync nature of the underlying function"""

name = "invalid-use-of-overload"
priority = -1
msgs = {
"C4765": (
"Do not mix async and synchronous overloads",
"invalid-use-of-overload",
"Functions and their overloads must be either all async or all synchronous.",
),
}

def visit_classdef(self, node):
"""Check that use of the @overload decorator matches the async/sync nature of the underlying function"""

# Obtain a list of all functions and function names
functions = []
try:
node.body
for item in node.body:
if hasattr(item, 'name'):
functions.append(item)

# Dictionary of lists of all functions by name
overloadedfunctions = {}
for item in functions:
if item.name in overloadedfunctions:
overloadedfunctions[item.name].append(item)
else:
overloadedfunctions[item.name] = [item]


# Loop through the overloaded functions and check they are the same type
for funct in overloadedfunctions.values():
if len(funct) > 1: # only need to check if there is more than 1 function with the same name
function_is_async = None

for item in funct:
if function_is_async is None:
function_is_async = self.is_function_async(item)

else:
if function_is_async != self.is_function_async(item):
self.add_message(
msgid=f"invalid-use-of-overload",
node=item,
confidence=None,
)
except:
pass


def is_function_async(self, node):
try:
str(node.__class__).index("Async")
return True
except:
if node.returns is None:
return False
try:
if node.returns.value.name == "Awaitable":
return True
else:
return False
except:
return False


class DoNotUseLegacyTyping(BaseChecker):

""" Rule to check that we aren't using legacy typing using comments. """
Expand Down Expand Up @@ -2835,7 +2918,6 @@ class DoNotImportAsyncio(BaseChecker):

name = "do-not-import-asyncio"
priority = -1
# TODO Find message number
msgs = {
"C4763": (
"Do not import the asyncio package directly in your library",
Expand Down Expand Up @@ -2864,8 +2946,6 @@ def visit_import(self, node):
)


# [Pylint] custom linter check for invalid use of @overload #3229


class DoNotLogExceptions(BaseChecker):

Expand Down Expand Up @@ -2940,8 +3020,83 @@ def check_for_logging(self, node, exception_name):


# [Pylint] Address Commented out Pylint Custom Plugin Checkers #3228
# [Pylint] Add a check for connection_verify hardcoded settings #35355
# [Pylint] Investigate pylint rule around missing dependency #3231


class DoNotHardcodeConnectionVerify(BaseChecker):

"""Rule to check that developers do not hardcode a boolean to connection_verify."""

name = "do-not-hardcode-connection-verify"
priority = -1
msgs = {
"C4767": (
"Do not hardcode a boolean value to connection_verify",
"do-not-hardcode-connection-verify",
"Do not hardcode a boolean value to connection_verify. It's up to customers who use the code to be able to set it",
),
}


def visit_call(self, node):
"""Visit function calls to ensure it isn't used as a parameter"""
try:
for keyword in node.keywords:
if keyword.arg == "connection_verify":
if type(keyword.value.value) == bool:
self.add_message(
msgid=f"do-not-hardcode-connection-verify",
node=keyword,
confidence=None,
)
except:
pass


def visit_assign(self, node):
"""Visiting variable Assignments"""
try: # Picks up self.connection_verify = True
if node.targets[0].attrname == "connection_verify":
if type(node.value.value) == bool:
self.add_message(
msgid=f"do-not-hardcode-connection-verify",
node=node,
confidence=None,
)
except:
try: # connection_verify = True
if node.targets[0].name == "connection_verify":
if type(node.value.value) == bool:
self.add_message(
msgid=f"do-not-hardcode-connection-verify",
node=node,
confidence=None,
)
except:
pass


def visit_annassign(self, node):
"""Visiting variable annotated assignments"""
try: # self.connection_verify: bool = True
if node.target.attrname == "connection_verify":
if type(node.value.value) == bool:
self.add_message(
msgid=f"do-not-hardcode-connection-verify",
node=node,
confidence=None,
)
except: # Picks up connection_verify: bool = True
try:
if node.target.name == "connection_verify":
if type(node.value.value) == bool:
self.add_message(
msgid=f"do-not-hardcode-connection-verify",
node=node,
confidence=None,
)
except:
pass



# if a linter is registered in this function then it will be checked with pylint
Expand Down Expand Up @@ -2977,22 +3132,20 @@ def register(linter):
linter.register_checker(DoNotImportAsyncio(linter))
linter.register_checker(NoLegacyAzureCoreHttpResponseImport(linter))
linter.register_checker(NoImportTypingFromTypeCheck(linter))
linter.register_checker(InvalidUseOfOverload(linter))
linter.register_checker(DoNotUseLegacyTyping(linter))
linter.register_checker(DoNotLogErrorsEndUpRaising(linter))
linter.register_checker(DoNotLogExceptions(linter))

# [Pylint] custom linter check for invalid use of @overload #3229
# [Pylint] Address Commented out Pylint Custom Plugin Checkers #3228
# [Pylint] Add a check for connection_verify hardcoded settings #35355
# [Pylint] Investigate pylint rule around missing dependency #3231
linter.register_checker(DoNotHardcodeConnectionVerify(linter))

# disabled by default, use pylint --enable=check-docstrings if you want to use it
linter.register_checker(CheckDocstringParameters(linter))


# Rules are disabled until false positive rate improved
# linter.register_checker(CheckForPolicyUse(linter))
# linter.register_checker(ClientHasApprovedMethodNamePrefix(linter))
linter.register_checker(ClientHasApprovedMethodNamePrefix(linter))

# linter.register_checker(ClientDocstringUsesLiteralIncludeForCodeExample(linter))
# linter.register_checker(ClientLROMethodsUseCorePolling(linter))
Expand Down
Loading

0 comments on commit e2dcbb9

Please sign in to comment.