From bf0f3e5138e3dd184fcb2ecae200aa1b51da295a Mon Sep 17 00:00:00 2001 From: PyBlend <13187637+MJoshuaB@users.noreply.github.com> Date: Mon, 7 Oct 2024 11:18:24 +1300 Subject: [PATCH] Revert "Approved prefix" --- .../azure-pylint-guidelines-checker/README.md | 7 +- .../pylint_guidelines_checker.py | 132 ++++---- .../client_has_approved_method_name_prefix.py | 169 ++++------ .../tests/test_pylint_custom_plugins.py | 319 +++++++++++------- 4 files changed, 315 insertions(+), 312 deletions(-) diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/README.md b/tools/pylint-extensions/azure-pylint-guidelines-checker/README.md index 8da3d1cece0..2078996a9d9 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/README.md +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/README.md @@ -95,10 +95,9 @@ In the case of a false positive, use the disable command to remove the pylint er | 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. | -| TODO | custom linter check for invalid use of @overload #3229 | | | | TODO | Custom Linter check for Exception Logging #3227 | | | -| 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 | Address Commented out Pylint Custom Plugin Checkers #3228 | | | | 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 | | | \ No newline at end of file + diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py index 73cc19c17c7..cd5effa2979 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py @@ -161,93 +161,79 @@ 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": "", + "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): - 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 + """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. - 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, + :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." ) + pass - def leave_classdef(self, node): - self.process_class = None class ClientMethodsUseKwargsWithMultipleParameters(BaseChecker): name = "client-method-multiple-parameters" @@ -3071,7 +3057,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)) diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_files/client_has_approved_method_name_prefix.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_files/client_has_approved_method_name_prefix.py index 88d34446060..36f94556327 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_files/client_has_approved_method_name_prefix.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_files/client_has_approved_method_name_prefix.py @@ -1,174 +1,129 @@ +from azure.core.tracing.decorator import distributed_trace + + # test_ignores_constructor -class ConstrClient(): #@ - def __init__(self, **kwargs): #@ +class SomeClient(): # @ + def __init__(self, **kwargs): # @ pass # test_ignores_private_method -class PrivClient(): #@ - def _private_method(self, **kwargs): #@ +class Some1Client(): # @ + def _private_method(self, **kwargs): # @ pass # test_ignores_if_exists_suffix -class ExistsClient(): #@ - def check_if_exists(self, **kwargs): #@ +class Some2Client(): # @ + def check_if_exists(self, **kwargs): # @ pass -# test_ignores_approved_prefix_names -class ApprovedClient(): #@ - def get_noun(self): #@ - pass - - def list_noun(self): #@ - pass - - def create_noun(self): #@ - pass - - def upsert_noun(self): #@ - pass - - def set_noun(self): #@ - pass - - def update_noun(self): #@ - pass - - def replace_noun(self): #@ +# test_ignores_from_prefix +class Some3Client(): # @ + def from_connection_string(self, **kwargs): # @ pass - - def append_noun(self): #@ - pass - - def add_noun(self): #@ - pass - - def delete_noun(self): #@ - pass - - def remove_noun(self): #@ - pass - - def begin_noun(self): #@ - pass - - def upload_noun(self): #@ + + +# test_ignores_approved_prefix_names +class Some4Client(): # @ + def create_configuration(self): # @ pass - - def download_noun(self): #@ + + def get_thing(self): # @ pass - - def close_noun(self): #@ + + def list_thing(self): # @ pass - - def cancel_noun(self): #@ + + def upsert_thing(self): # @ pass - - def clear_noun(self): #@ + + def set_thing(self): # @ pass - - def subscribe_noun(self): #@ + + def update_thing(self): # @ pass - - def send_noun(self): #@ + + def replace_thing(self): # @ pass - - def query_noun(self): #@ + + def append_thing(self): # @ pass - - def analyze_noun(self): #@ + + def add_thing(self): # @ pass - - def train_noun(self): #@ + + def delete_thing(self): # @ pass - - def detect_noun(self): #@ + + def remove_thing(self): # @ pass - - def from_noun(self): #@ + + def begin_thing(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 NestedClient(): #@ - def create_configuration(self, **kwargs): #@ - def nested(hello, world): #@ +class Some5Client(): # @ + def create_configuration(self, **kwargs): # @ + def nested(hello, world): pass # test_finds_unapproved_prefix_names -class UnapprovedClient(): #@ - def build_configuration(self): #@ +class Some6Client(): # @ + @distributed_trace + 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 \ No newline at end of file diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py index f92dd424fdb..77eb48f1f47 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/tests/test_pylint_custom_plugins.py @@ -12,7 +12,6 @@ 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__, "..")) @@ -303,131 +302,202 @@ def test_guidelines_link_active(self): 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): - trees = _load_file("client_has_approved_method_name_prefix.py") - return {tree[0].name:tree for tree in trees} + 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 - @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_module(mod) - self.checker.visit_classdef(cls) - self.checker.visit_functiondef(func) - self.checker.leave_classdef(cls) - - def test_ignores_private_method(self, setup, modules): - mod = modules["public"] - cls, func = setup.get("PrivClient") - 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_if_exists_suffix(self, setup, modules): - mod = modules["public"] - cls, func = setup.get("ExistsClient") - 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_approved_prefix_names(self, setup, modules): - mod = modules["public"] - cls, *funcs = setup.get("ApprovedClient") - with self.assertNoMessages(): - 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_non_client_with_unapproved_prefix_names(self, setup, modules): - mod = modules["public"] - cls, func = setup.get("SomethingElse") - 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_nested_function_with_unapproved_prefix_names(self, setup, modules): - mod = modules["public"] - cls, func, nested = setup.get("NestedClient") - with self.assertNoMessages(): - 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_finds_unapproved_prefix_names(self, setup, modules): - mod = modules["public"] - cls, *funcs = setup.get("UnapprovedClient") - msgs = [ + def test_ignores_constructor(self, setup): + class_node = setup.body[1] + with self.assertNoMessages(): + self.checker.visit_classdef(class_node) + + def test_ignores_private_method(self, setup): + class_node = setup.body[2] + with self.assertNoMessages(): + self.checker.visit_classdef(class_node) + + def test_ignores_if_exists_suffix(self, setup): + class_node = setup.body[3] + with self.assertNoMessages(): + self.checker.visit_classdef(class_node) + + def test_ignores_from_prefix(self, setup): + class_node = setup.body[4] + with self.assertNoMessages(): + self.checker.visit_classdef(class_node) + + def test_ignores_approved_prefix_names(self, setup): + class_node = setup.body[5] + with self.assertNoMessages(): + self.checker.visit_classdef(class_node) + + def test_ignores_non_client_with_unapproved_prefix_names(self, setup): + class_node = setup.body[6] + with self.assertNoMessages(): + self.checker.visit_classdef(class_node) + + def test_ignores_nested_function_with_unapproved_prefix_names(self, setup): + class_node = setup.body[7] + with self.assertNoMessages(): + self.checker.visit_classdef(class_node) + + 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=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_module(mod) - self.checker.visit_classdef(cls) - self.checker.visit_functiondef(func) - self.checker.leave_classdef(cls) - - 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) + 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_guidelines_link_active(self): url = "https://azure.github.io/azure-sdk/python_design.html#service-operations" @@ -1995,10 +2065,7 @@ def test_package_name_violation(self, setup): 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) @@ -2040,10 +2107,7 @@ def test_client_suffix_violation(self, setup): 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) @@ -2930,6 +2994,7 @@ def test_begin_delete_operation_correct_return(self, setup): class TestDocstringParameters(pylint.testutils.CheckerTestCase): + """Test that we are checking the docstring is correct""" CHECKER_CLASS = checker.CheckDocstringParameters @@ -3629,8 +3694,6 @@ def function(x): #@ with self.assertNoMessages(): self.checker.visit_functiondef(fdef) - -# [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