Skip to content

Commit 6c5d466

Browse files
MJoshuaBAlirose BurrellJessicaBell0016234397
authored
[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 6d262e3, reversing changes made to 65e431f. * 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>
1 parent d99e90c commit 6c5d466

4 files changed

Lines changed: 321 additions & 314 deletions

File tree

‎tools/pylint-extensions/azure-pylint-guidelines-checker/README.md‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,10 @@ In the case of a false positive, use the disable command to remove the pylint er
9494
| 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) |
9595
| no-typing-import-in-type-check | Do not import typing under TYPE_CHECKING. | pylint:disable=no-typing-import-in-type-check | No Link. |
9696
| 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. |
97-
| 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.
97+
| 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. |
9898
| do-not-import-asyncio | Do not import asyncio directly. | pylint:disable=do-not-import-asyncio | No Link. |
99-
| 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) |
99+
| TODO | custom linter check for invalid use of @overload #3229 | | |
100+
| 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) |
101+
| 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) |
102+
| TODO | Add a check for connection_verify hardcoded settings #35355 | | |
103+
| TODO | Address Commented out Pylint Custom Plugin Checkers #3228 | | |

‎tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py‎

Lines changed: 79 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -161,79 +161,93 @@ class ClientHasApprovedMethodNamePrefix(BaseChecker):
161161
" https://azure.github.io/azure-sdk/python_design.html#service-operations",
162162
"unapproved-client-method-name-prefix",
163163
"All clients should use the preferred verbs for method names.",
164-
)
165-
}
166-
options = (
167-
(
168-
"ignore-unapproved-client-method-name-prefix",
169-
{
170-
"default": False,
171-
"type": "yn",
172-
"metavar": "<y_or_n>",
173-
"help": "Allow clients to not use preferred method name prefixes",
174-
},
175164
),
176-
)
165+
}
177166

178-
ignore_clients = [
167+
ignore_clients = [
179168
"PipelineClient",
180169
"AsyncPipelineClient",
181170
"ARMPipelineClient",
182171
"AsyncARMPipelineClient",
183172
]
184173

174+
approved_prefixes = [
175+
"get",
176+
"list",
177+
"create",
178+
"upsert",
179+
"set",
180+
"update",
181+
"replace",
182+
"append",
183+
"add",
184+
"delete",
185+
"remove",
186+
"begin",
187+
"upload",
188+
"download", # standard verbs
189+
"close", # very common verb
190+
"cancel",
191+
"clear",
192+
"subscribe",
193+
"send",
194+
"query", # common verbs
195+
"analyze",
196+
"train",
197+
"detect", # future proofing
198+
"from", # special case
199+
]
200+
201+
ignored_decorators = [
202+
"property",
203+
]
204+
185205
def __init__(self, linter=None):
186206
super(ClientHasApprovedMethodNamePrefix, self).__init__(linter)
207+
self.process_class = None
208+
self.namespace = None
209+
210+
def _check_decorators(self, node):
211+
if not node.decorators:
212+
return False
213+
for decorator in node.decorators.nodes:
214+
if isinstance(decorator, astroid.nodes.Name) and decorator.name in self.ignored_decorators:
215+
return True
216+
return False
217+
218+
def visit_module(self, node):
219+
self.namespace = node.name
187220

188221
def visit_classdef(self, node):
189-
"""Visits every class in file and checks if it is a client. If it is a client, checks
190-
that approved method name prefixes are present.
222+
if all((
223+
node.name.endswith("Client"),
224+
node.name not in self.ignore_clients,
225+
not node.name.startswith("_"),
226+
not '._' in self.namespace,
227+
)):
228+
self.process_class = node
191229

192-
:param node: class node
193-
:type node: ast.ClassDef
194-
:return: None
195-
"""
196-
try:
197-
if node.name.endswith("Client") and node.name not in self.ignore_clients:
198-
client_methods = [
199-
child for child in node.get_children() if child.is_function
200-
]
201-
202-
approved_prefixes = [
203-
"get",
204-
"list",
205-
"create",
206-
"upsert",
207-
"set",
208-
"update",
209-
"replace",
210-
"append",
211-
"add",
212-
"delete",
213-
"remove",
214-
"begin",
215-
]
216-
for idx, method in enumerate(client_methods):
217-
if (
218-
method.name.startswith("__")
219-
or "_exists" in method.name
220-
or method.name.startswith("_")
221-
or method.name.startswith("from")
222-
):
223-
continue
224-
prefix = method.name.split("_")[0]
225-
if prefix.lower() not in approved_prefixes:
226-
self.add_message(
227-
msgid="unapproved-client-method-name-prefix",
228-
node=client_methods[idx],
229-
confidence=None,
230-
)
231-
except AttributeError:
232-
logger.debug(
233-
"Pylint custom checker failed to check if client has approved method name prefix."
230+
def visit_functiondef(self, node):
231+
if any((
232+
self.process_class is None, # not in a client class
233+
node.name.startswith("_"), # private method
234+
node.name.endswith("_exists"), # special case
235+
self._check_decorators(node), # property
236+
node.parent != self.process_class, # nested method
237+
)):
238+
return
239+
240+
# check for approved prefix
241+
parts = node.name.split("_")
242+
if parts[0].lower() not in self.approved_prefixes:
243+
self.add_message(
244+
msgid="unapproved-client-method-name-prefix",
245+
node=node,
246+
confidence=None,
234247
)
235-
pass
236248

249+
def leave_classdef(self, node):
250+
self.process_class = None
237251

238252
class ClientMethodsUseKwargsWithMultipleParameters(BaseChecker):
239253
name = "client-method-multiple-parameters"
@@ -2803,6 +2817,12 @@ def visit_import(self, node):
28032817
)
28042818
except:
28052819
pass
2820+
# [Pylint] custom linter check for invalid use of @overload #3229
2821+
# [Pylint] Custom Linter check for Exception Logging #3227
2822+
# [Pylint] Address Commented out Pylint Custom Plugin Checkers #3228
2823+
# [Pylint] Add a check for connection_verify hardcoded settings #35355
2824+
# [Pylint] Refactor test suite for custom pylint checkers to use files instead of docstrings #3233
2825+
# [Pylint] Investigate pylint rule around missing dependency #3231
28062826

28072827

28082828
class DoNotUseLegacyTyping(BaseChecker):
@@ -2991,7 +3011,7 @@ def register(linter):
29913011

29923012
# Rules are disabled until false positive rate improved
29933013
# linter.register_checker(CheckForPolicyUse(linter))
2994-
# linter.register_checker(ClientHasApprovedMethodNamePrefix(linter))
3014+
linter.register_checker(ClientHasApprovedMethodNamePrefix(linter))
29953015

29963016
# linter.register_checker(ClientDocstringUsesLiteralIncludeForCodeExample(linter))
29973017
# linter.register_checker(ClientLROMethodsUseCorePolling(linter))

0 commit comments

Comments
 (0)
⚡