diff --git a/tools/pylint-extensions/azure-pylint-guidelines-checker/README.md b/tools/pylint-extensions/azure-pylint-guidelines-checker/README.md index bf5bbe505..d06ab5a24 100644 --- a/tools/pylint-extensions/azure-pylint-guidelines-checker/README.md +++ b/tools/pylint-extensions/azure-pylint-guidelines-checker/README.md @@ -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 (<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) | \ No newline at end of file +| 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 | | | \ 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 da7dcfde0..20ba21669 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,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": "", - "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)) 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 36f945563..88d344460 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,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 \ 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 2cd88fbf1..c5423829f 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,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 +