[Pylint] Check client methods have approved name prefix (#9081)

* add placeholders

* add new placeholder comments

* exploring AST unfinished - minor emergency had to leave

* identifying some mismatched functions

* refactor checker and tests

* fix error with non-builtins decorators

* fine tuning and testing required

* add pylint report

* add ranked listing of reports

* format report as table

* add new verbs

* update report

* update reportcounts.md

* fix formatting for reportcounts.md

* update reportcounts.md

* minimal tests added

* Final Refactor

* not running multiple times picking up on different function types

* add TODOs

* removed code not reached

* Checks at a class level

* Looking into different connection_verify assignments

* Placeholders added back

* Checker base done

* exclue private namespaces and classes

* update reports

* Checker, Tests, & Readme done

* add new prefixes

* update unit tests

* remove reports

* remove commented code

* add checker to README

* Tidy Up

* Revert "Merge branch 'working-main' into approved_prefix"

This reverts commit 6d262e3c5c, reversing
changes made to 65e431f621.

* remove duplicate tests

---------

Co-authored-by: Alirose Burrell <16234397@massey.ac.nz>
Co-authored-by: JessicaBell00 <110268278+JessicaBell00@users.noreply.github.com>
Co-authored-by: 16234397 <110712668+16234397@users.noreply.github.com>
This commit is contained in:
PyBlend 2024-10-12 06:32:31 +13:00 коммит произвёл GitHub
Родитель d99e90ceca
Коммит 6c5d46658a
Не найден ключ, соответствующий данной подписи
Идентификатор ключа GPG: B5690EEEBB952194
4 изменённых файлов: 309 добавлений и 302 удалений

Просмотреть файл

@ -94,6 +94,10 @@ In the case of a false positive, use the disable command to remove the pylint er
| 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. |
| 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 (&lt;Python 3.8) type hinting comments | pylint:disable=do-not-use-legacy-typing | No Link.
| do-not-use-legacy-typing | Do not use legacy (&lt;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) |
| TODO | custom linter check for invalid use of @overload #3229 | | |
| 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) |
| TODO | Add a check for connection_verify hardcoded settings #35355 | | |
| TODO | Address Commented out Pylint Custom Plugin Checkers #3228 | | |

Просмотреть файл

@ -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"
@ -2803,6 +2817,12 @@ class NoImportTypingFromTypeCheck(BaseChecker):
)
except:
pass
# [Pylint] custom linter check for invalid use of @overload #3229
# [Pylint] Custom Linter check for Exception Logging #3227
# [Pylint] Address Commented out Pylint Custom Plugin Checkers #3228
# [Pylint] Add a check for connection_verify hardcoded settings #35355
# [Pylint] Refactor test suite for custom pylint checkers to use files instead of docstrings #3233
# [Pylint] Investigate pylint rule around missing dependency #3231
class DoNotUseLegacyTyping(BaseChecker):
@ -2991,7 +3011,7 @@ def register(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))

Просмотреть файл

@ -1,129 +1,174 @@
from azure.core.tracing.decorator import distributed_trace
# test_ignores_constructor
class SomeClient(): # @
def __init__(self, **kwargs): # @
class ConstrClient(): #@
def __init__(self, **kwargs): #@
pass
# test_ignores_private_method
class Some1Client(): # @
def _private_method(self, **kwargs): # @
class PrivClient(): #@
def _private_method(self, **kwargs): #@
pass
# test_ignores_if_exists_suffix
class Some2Client(): # @
def check_if_exists(self, **kwargs): # @
pass
# test_ignores_from_prefix
class Some3Client(): # @
def from_connection_string(self, **kwargs): # @
class ExistsClient(): #@
def check_if_exists(self, **kwargs): #@
pass
# test_ignores_approved_prefix_names
class Some4Client(): # @
def create_configuration(self): # @
class ApprovedClient(): #@
def get_noun(self): #@
pass
def get_thing(self): # @
def list_noun(self): #@
pass
def list_thing(self): # @
def create_noun(self): #@
pass
def upsert_thing(self): # @
def upsert_noun(self): #@
pass
def set_thing(self): # @
def set_noun(self): #@
pass
def update_thing(self): # @
def update_noun(self): #@
pass
def replace_thing(self): # @
def replace_noun(self): #@
pass
def append_thing(self): # @
def append_noun(self): #@
pass
def add_thing(self): # @
def add_noun(self): #@
pass
def delete_thing(self): # @
def delete_noun(self): #@
pass
def remove_thing(self): # @
def remove_noun(self): #@
pass
def begin_thing(self): # @
def begin_noun(self): #@
pass
def upload_noun(self): #@
pass
def download_noun(self): #@
pass
def close_noun(self): #@
pass
def cancel_noun(self): #@
pass
def clear_noun(self): #@
pass
def subscribe_noun(self): #@
pass
def send_noun(self): #@
pass
def query_noun(self): #@
pass
def analyze_noun(self): #@
pass
def train_noun(self): #@
pass
def detect_noun(self): #@
pass
def from_noun(self): #@
pass
# test_ignores_non_client_with_unapproved_prefix_names
class SomethingElse(): # @
def download_thing(self, some, **kwargs): # @
class SomethingElse(): #@
def download_thing(self, some, **kwargs): #@
pass
# test_ignores_nested_function_with_unapproved_prefix_names
class Some5Client(): # @
def create_configuration(self, **kwargs): # @
def nested(hello, world):
class NestedClient(): #@
def create_configuration(self, **kwargs): #@
def nested(hello, world): #@
pass
# test_finds_unapproved_prefix_names
class Some6Client(): # @
@distributed_trace
def build_configuration(self): # @
class UnapprovedClient(): #@
def build_configuration(self): #@
pass
def generate_thing(self): # @
def generate_thing(self): #@
pass
def make_thing(self): # @
def make_thing(self): #@
pass
def insert_thing(self): # @
def insert_thing(self): #@
pass
def put_thing(self): # @
def put_thing(self): #@
pass
def creates_configuration(self): # @
def creates_configuration(self): #@
pass
def gets_thing(self): # @
def gets_thing(self): #@
pass
def lists_thing(self): # @
def lists_thing(self): #@
pass
def upserts_thing(self): # @
def upserts_thing(self): #@
pass
def sets_thing(self): # @
def sets_thing(self): #@
pass
def updates_thing(self): # @
def updates_thing(self): #@
pass
def replaces_thing(self): # @
def replaces_thing(self): #@
pass
def appends_thing(self): # @
def appends_thing(self): #@
pass
def adds_thing(self): # @
def adds_thing(self): #@
pass
def deletes_thing(self): # @
def deletes_thing(self): #@
pass
def removes_thing(self): # @
def removes_thing(self): #@
pass
# test_ignores_property
class PropClient(): #@
@property
def func(self): #@
pass
# test_ignores_private_client
class _PrivateClient(): #@
def get_thing(self): #@
pass
# test_ignores_private_module
class PrivateModuleClient(): #@
def get_thing(self): #@
pass

Просмотреть файл

@ -12,6 +12,7 @@ import os
from azure.core import PipelineClient
from azure.core.configuration import Configuration
import pylint_guidelines_checker as checker
from pylint.testutils import MessageTest
TEST_FOLDER = os.path.abspath(os.path.join(__file__, ".."))
@ -302,202 +303,131 @@ class TestClientsDoNotUseStaticMethods(pylint.testutils.CheckerTestCase):
assert response.http_response.status_code == 200
def _load_file(filename):
file_path = os.path.join(TEST_FOLDER, "test_files", filename)
with open(file_path, "r") as file:
contents = file.read().split("\n\n\n") # Split by triple newline (2 blank lines)
return [astroid.extract_node(content) for content in contents]
class TestClientHasApprovedMethodNamePrefix(pylint.testutils.CheckerTestCase):
CHECKER_CLASS = checker.ClientHasApprovedMethodNamePrefix
@pytest.fixture(scope="class")
def setup(self):
file = open(
os.path.join(TEST_FOLDER, "test_files", "client_has_approved_method_name_prefix.py")
)
node = astroid.parse(file.read())
file.close()
return node
trees = _load_file("client_has_approved_method_name_prefix.py")
return {tree[0].name:tree for tree in trees}
def test_ignores_constructor(self, setup):
class_node = setup.body[1]
@pytest.fixture(scope="class")
def modules(self):
mods = {
"public":astroid.nodes.Module(name="azure.service.subservice.operations"),
"private":astroid.nodes.Module(name="azure.mgmt._generated.operations"),
}
return mods
def test_ignores_constructor(self, setup, modules):
mod = modules["public"]
cls, func = setup.get("ConstrClient")
with self.assertNoMessages():
self.checker.visit_classdef(class_node)
self.checker.visit_module(mod)
self.checker.visit_classdef(cls)
self.checker.visit_functiondef(func)
self.checker.leave_classdef(cls)
def test_ignores_private_method(self, setup):
class_node = setup.body[2]
def test_ignores_private_method(self, setup, modules):
mod = modules["public"]
cls, func = setup.get("PrivClient")
with self.assertNoMessages():
self.checker.visit_classdef(class_node)
self.checker.visit_module(mod)
self.checker.visit_classdef(cls)
self.checker.visit_functiondef(func)
self.checker.leave_classdef(cls)
def test_ignores_if_exists_suffix(self, setup):
class_node = setup.body[3]
def test_ignores_if_exists_suffix(self, setup, modules):
mod = modules["public"]
cls, func = setup.get("ExistsClient")
with self.assertNoMessages():
self.checker.visit_classdef(class_node)
self.checker.visit_module(mod)
self.checker.visit_classdef(cls)
self.checker.visit_functiondef(func)
self.checker.leave_classdef(cls)
def test_ignores_from_prefix(self, setup):
class_node = setup.body[4]
def test_ignores_approved_prefix_names(self, setup, modules):
mod = modules["public"]
cls, *funcs = setup.get("ApprovedClient")
with self.assertNoMessages():
self.checker.visit_classdef(class_node)
self.checker.visit_module(mod)
self.checker.visit_classdef(cls)
for func in funcs:
self.checker.visit_functiondef(func)
self.checker.leave_classdef(cls)
def test_ignores_approved_prefix_names(self, setup):
class_node = setup.body[5]
def test_ignores_non_client_with_unapproved_prefix_names(self, setup, modules):
mod = modules["public"]
cls, func = setup.get("SomethingElse")
with self.assertNoMessages():
self.checker.visit_classdef(class_node)
self.checker.visit_module(mod)
self.checker.visit_classdef(cls)
self.checker.visit_functiondef(func)
self.checker.leave_classdef(cls)
def test_ignores_non_client_with_unapproved_prefix_names(self, setup):
class_node = setup.body[6]
def test_ignores_nested_function_with_unapproved_prefix_names(self, setup, modules):
mod = modules["public"]
cls, func, nested = setup.get("NestedClient")
with self.assertNoMessages():
self.checker.visit_classdef(class_node)
self.checker.visit_module(mod)
self.checker.visit_classdef(cls)
self.checker.visit_functiondef(func)
self.checker.visit_functiondef(nested)
self.checker.leave_classdef(cls)
def test_ignores_nested_function_with_unapproved_prefix_names(self, setup):
class_node = setup.body[7]
def test_finds_unapproved_prefix_names(self, setup, modules):
mod = modules["public"]
cls, *funcs = setup.get("UnapprovedClient")
msgs = [
pylint.testutils.MessageTest(
msg_id="unapproved-client-method-name-prefix",
line=func.position.lineno,
node=func,
col_offset=func.position.col_offset,
end_line=func.position.end_lineno,
end_col_offset=func.position.end_col_offset,
) for func in funcs
]
with self.assertAddsMessages(*msgs):
self.checker.visit_module(mod)
self.checker.visit_classdef(cls)
for func in funcs:
self.checker.visit_functiondef(func)
self.checker.leave_classdef(cls)
def test_ignores_property(self, setup, modules):
mod = modules["public"]
cls, func = setup.get("PropClient")
with self.assertNoMessages():
self.checker.visit_classdef(class_node)
self.checker.visit_module(mod)
self.checker.visit_classdef(cls)
self.checker.visit_functiondef(func)
self.checker.leave_classdef(cls)
def test_finds_unapproved_prefix_names(self, setup):
class_node = setup.body[8]
func_node_a = setup.body[8].body[0]
func_node_b = setup.body[8].body[1]
func_node_c = setup.body[8].body[2]
func_node_d = setup.body[8].body[3]
func_node_e = setup.body[8].body[4]
func_node_f = setup.body[8].body[5]
func_node_g = setup.body[8].body[6]
func_node_h = setup.body[8].body[7]
func_node_i = setup.body[8].body[8]
func_node_j = setup.body[8].body[9]
func_node_k = setup.body[8].body[10]
func_node_l = setup.body[8].body[11]
func_node_m = setup.body[8].body[12]
func_node_n = setup.body[8].body[13]
func_node_o = setup.body[8].body[14]
func_node_p = setup.body[8].body[15]
with self.assertAddsMessages(
pylint.testutils.MessageTest(
msg_id="unapproved-client-method-name-prefix",
line=83,
node=func_node_a,
col_offset=4,
end_line=83,
end_col_offset=27,
),
pylint.testutils.MessageTest(
msg_id="unapproved-client-method-name-prefix",
line=86,
node=func_node_b,
col_offset=4,
end_line=86,
end_col_offset=22,
),
pylint.testutils.MessageTest(
msg_id="unapproved-client-method-name-prefix",
line=89,
node=func_node_c,
col_offset=4,
end_line=89,
end_col_offset=18,
),
pylint.testutils.MessageTest(
msg_id="unapproved-client-method-name-prefix",
line=92,
node=func_node_d,
col_offset=4,
end_line=92,
end_col_offset=20,
),
pylint.testutils.MessageTest(
msg_id="unapproved-client-method-name-prefix",
line=95,
node=func_node_e,
col_offset=4,
end_line=95,
end_col_offset=17,
),
pylint.testutils.MessageTest(
msg_id="unapproved-client-method-name-prefix",
line=98,
node=func_node_f,
col_offset=4,
end_line=98,
end_col_offset=29,
),
pylint.testutils.MessageTest(
msg_id="unapproved-client-method-name-prefix",
line=101,
node=func_node_g,
col_offset=4,
end_line=101,
end_col_offset=18,
),
pylint.testutils.MessageTest(
msg_id="unapproved-client-method-name-prefix",
line=104,
node=func_node_h,
col_offset=4,
end_line=104,
end_col_offset=19,
),
pylint.testutils.MessageTest(
msg_id="unapproved-client-method-name-prefix",
line=107,
node=func_node_i,
col_offset=4,
end_line=107,
end_col_offset=21,
),
pylint.testutils.MessageTest(
msg_id="unapproved-client-method-name-prefix",
line=110,
node=func_node_j,
col_offset=4,
end_line=110,
end_col_offset=18,
),
pylint.testutils.MessageTest(
msg_id="unapproved-client-method-name-prefix",
line=113,
node=func_node_k,
col_offset=4,
end_line=113,
end_col_offset=21,
),
pylint.testutils.MessageTest(
msg_id="unapproved-client-method-name-prefix",
line=116,
node=func_node_l,
col_offset=4,
end_line=116,
end_col_offset=22,
),
pylint.testutils.MessageTest(
msg_id="unapproved-client-method-name-prefix",
line=119,
node=func_node_m,
col_offset=4,
end_line=119,
end_col_offset=21,
),
pylint.testutils.MessageTest(
msg_id="unapproved-client-method-name-prefix",
line=122,
node=func_node_n,
col_offset=4,
end_line=122,
end_col_offset=18,
),
pylint.testutils.MessageTest(
msg_id="unapproved-client-method-name-prefix",
line=125,
node=func_node_o,
col_offset=4,
end_line=125,
end_col_offset=21,
),
pylint.testutils.MessageTest(
msg_id="unapproved-client-method-name-prefix",
line=128,
node=func_node_p,
col_offset=4,
end_line=128,
end_col_offset=21,
),
):
self.checker.visit_classdef(class_node)
def test_ignores_private_client(self, setup, modules):
mod = modules["public"]
cls, func = setup.get("_PrivateClient")
with self.assertNoMessages():
self.checker.visit_module(mod)
self.checker.visit_classdef(cls)
self.checker.visit_functiondef(func)
self.checker.leave_classdef(cls)
def test_ignores_private_module(self, setup, modules):
mod = modules["private"]
cls, func = setup.get("PrivateModuleClient")
with self.assertNoMessages():
self.checker.visit_module(mod)
self.checker.visit_classdef(cls)
self.checker.visit_functiondef(func)
self.checker.leave_classdef(cls)
def test_guidelines_link_active(self):
url = "https://azure.github.io/azure-sdk/python_design.html#service-operations"
@ -2065,7 +1995,10 @@ class TestPackageNameDoesNotUseUnderscoreOrPeriod(pylint.testutils.CheckerTestCa
module_node.body = [package_name]
with self.assertAddsMessages(
pylint.testutils.MessageTest(
msg_id="package-name-incorrect", line=0, node=module_node, col_offset=0,
msg_id="package-name-incorrect",
line=0,
node=module_node,
col_offset=0,
)
):
self.checker.visit_module(module_node)
@ -2107,7 +2040,10 @@ class TestServiceClientUsesNameWithClientSuffix(pylint.testutils.CheckerTestCase
module_node.body = [client_node]
with self.assertAddsMessages(
pylint.testutils.MessageTest(
msg_id="client-suffix-needed", line=0, node=module_node, col_offset=0,
msg_id="client-suffix-needed",
line=0,
node=module_node,
col_offset=0,
)
):
self.checker.visit_module(module_node)
@ -2994,7 +2930,6 @@ class TestDeleteOperationReturnType(pylint.testutils.CheckerTestCase):
class TestDocstringParameters(pylint.testutils.CheckerTestCase):
"""Test that we are checking the docstring is correct"""
CHECKER_CLASS = checker.CheckDocstringParameters
@ -3597,6 +3532,7 @@ class TestDoNotLogErrorsEndUpRaising(pylint.testutils.CheckerTestCase):
):
self.checker.visit_try(try_node)
# [Pylint] custom linter check for invalid use of @overload #3229
@ -3789,4 +3725,6 @@ class TestDoNotLogExceptions(pylint.testutils.CheckerTestCase):
# [Pylint] Address Commented out Pylint Custom Plugin Checkers #3228
# [Pylint] Add a check for connection_verify hardcoded settings #35355
# [Pylint] Refactor test suite for custom pylint checkers to use files instead of docstrings #3233
# [Pylint] Investigate pylint rule around missing dependency #3231