From 7a120ab2f57e6d4131385d23f4d670028d827c99 Mon Sep 17 00:00:00 2001 From: noidname01 <55401762+noidname01@users.noreply.github.com> Date: Mon, 6 May 2024 13:56:40 +0800 Subject: [PATCH] [#3203] improvement(PyClient): Add pylint Rules for Python client (#3279) ### What changes were proposed in this pull request? * Add pylint rule for logging, logging should use old format (%), not fstring * Unify all the logging format * Set many disable rules for future modification (I add TODO tags after them, will work on them after this PR) ### Why are the changes needed? Fix: #3203 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? ```bash ./gradlew :clients:client-python:test ``` Co-authored-by: TimWang --- clients/client-python/.gitignore | 2 + clients/client-python/build.gradle.kts | 8 ++- .../gravitino/api/supports_schemas.py | 2 +- .../client/gravitino_admin_client.py | 8 +-- .../gravitino/client/gravitino_client_base.py | 4 +- .../gravitino/client/gravitino_metalake.py | 7 +-- .../gravitino/dto/dto_converters.py | 26 +++++---- .../dto/requests/fileset_updates_request.py | 2 +- .../dto/responses/entity_list_response.py | 2 +- .../dto/responses/schema_response.py | 4 +- .../exceptions/gravitino_runtime_exception.py | 1 - .../illegal_name_identifier_exception.py | 4 +- .../exceptions/illegal_namespace_exception.py | 4 +- .../exceptions/no_such_metalake_exception.py | 3 - .../exceptions/not_found_exception.py | 3 - .../gravitino/name_identifier.py | 3 +- .../gravitino/utils/http_client.py | 11 ++-- clients/client-python/pylintrc | 55 +++++++++++++++++++ clients/client-python/requirements-dev.txt | 3 +- .../tests/integration/integration_test_env.py | 15 ++--- .../tests/integration/test_metalake.py | 4 +- 21 files changed, 113 insertions(+), 58 deletions(-) create mode 100644 clients/client-python/pylintrc diff --git a/clients/client-python/.gitignore b/clients/client-python/.gitignore index 4399cbb1a8d..03a9c5f2e91 100644 --- a/clients/client-python/.gitignore +++ b/clients/client-python/.gitignore @@ -5,5 +5,7 @@ **/__pycache__/** gravitino.egg-info vevn +venv .vevn +.venv .idea diff --git a/clients/client-python/build.gradle.kts b/clients/client-python/build.gradle.kts index 03bf90f687a..16e1a832d8a 100644 --- a/clients/client-python/build.gradle.kts +++ b/clients/client-python/build.gradle.kts @@ -38,6 +38,11 @@ tasks { args = listOf("install", "-e", ".[dev]") } + val pylint by registering(VenvTask::class) { + venvExec = "pylint" + args = listOf("./gravitino", "./tests") + } + val test by registering(VenvTask::class) { val skipPyClientITs = project.hasProperty("skipPyClientITs") if (!skipPyClientITs) { @@ -45,7 +50,7 @@ tasks { gravitinoServer("start") } - dependsOn(pipInstall) + dependsOn(pipInstall, pylint) venvExec = "python" args = listOf("-m", "unittest") workingDir = projectDir.resolve(".") @@ -62,6 +67,7 @@ tasks { } val build by registering(VenvTask::class) { + dependsOn(pipInstall, pylint) } val clean by registering(Delete::class) { diff --git a/clients/client-python/gravitino/api/supports_schemas.py b/clients/client-python/gravitino/api/supports_schemas.py index dfd5092db76..d237b03b8ce 100644 --- a/clients/client-python/gravitino/api/supports_schemas.py +++ b/clients/client-python/gravitino/api/supports_schemas.py @@ -3,7 +3,7 @@ This software is licensed under the Apache License version 2. """ from abc import ABC, abstractmethod -from typing import List, Dict, Optional +from typing import List, Dict from gravitino.api.schema import Schema from gravitino.api.schema_change import SchemaChange diff --git a/clients/client-python/gravitino/client/gravitino_admin_client.py b/clients/client-python/gravitino/client/gravitino_admin_client.py index 844b3e12a5a..f3ea30681fb 100644 --- a/clients/client-python/gravitino/client/gravitino_admin_client.py +++ b/clients/client-python/gravitino/client/gravitino_admin_client.py @@ -8,7 +8,6 @@ from gravitino.client.gravitino_client_base import GravitinoClientBase from gravitino.client.gravitino_metalake import GravitinoMetalake from gravitino.dto.dto_converters import DTOConverters -from gravitino.dto.metalake_dto import MetalakeDTO from gravitino.dto.requests.metalake_create_request import MetalakeCreateRequest from gravitino.dto.requests.metalake_updates_request import MetalakeUpdatesRequest from gravitino.dto.responses.drop_response import DropResponse @@ -26,8 +25,7 @@ class GravitinoAdminClient(GravitinoClientBase): Normal users should use {@link GravitinoClient} to connect with the Gravitino server. """ - def __init__(self, uri): # TODO: AuthDataProvider authDataProvider - super().__init__(uri) + # TODO: AuthDataProvider authDataProvider def list_metalakes(self) -> List[GravitinoMetalake]: """Retrieves a list of Metalakes from the Gravitino API. @@ -106,6 +104,6 @@ def drop_metalake(self, ident: NameIdentifier) -> bool: dropResponse = DropResponse.from_json(resp.body, infer_missing=True) return dropResponse.dropped() - except Exception as e: - logger.warning(f"Failed to drop metalake {ident}") + except Exception: + logger.warning("Failed to drop metalake %s", ident) return False diff --git a/clients/client-python/gravitino/client/gravitino_client_base.py b/clients/client-python/gravitino/client/gravitino_client_base.py index 92b0932d09f..affeec08ee7 100644 --- a/clients/client-python/gravitino/client/gravitino_client_base.py +++ b/clients/client-python/gravitino/client/gravitino_client_base.py @@ -8,7 +8,7 @@ from gravitino.client.gravitino_version import GravitinoVersion from gravitino.dto.responses.metalake_response import MetalakeResponse from gravitino.name_identifier import NameIdentifier -from gravitino.utils import HTTPClient, Response +from gravitino.utils import HTTPClient logger = logging.getLogger(__name__) @@ -69,4 +69,4 @@ def close(self): try: self._rest_client.close() except Exception as e: - logger.warning("Failed to close the HTTP REST client", e) + logger.warning("Failed to close the HTTP REST client: %s", e) diff --git a/clients/client-python/gravitino/client/gravitino_metalake.py b/clients/client-python/gravitino/client/gravitino_metalake.py index 95b3742af89..95fdf41713e 100644 --- a/clients/client-python/gravitino/client/gravitino_metalake.py +++ b/clients/client-python/gravitino/client/gravitino_metalake.py @@ -3,10 +3,10 @@ This software is licensed under the Apache License version 2. """ import logging +from typing import List, Dict from gravitino.api.catalog import Catalog from gravitino.api.catalog_change import CatalogChange -from gravitino.dto.audit_dto import AuditDTO from gravitino.dto.dto_converters import DTOConverters from gravitino.dto.metalake_dto import MetalakeDTO from gravitino.dto.requests.catalog_create_request import CatalogCreateRequest @@ -19,7 +19,6 @@ from gravitino.namespace import Namespace from gravitino.utils import HTTPClient -from typing import List, Dict logger = logging.getLogger(__name__) @@ -190,6 +189,6 @@ def drop_catalog(self, ident: NameIdentifier) -> bool: drop_response.validate() return drop_response.dropped() - except Exception as e: - logger.warning(f"Failed to drop catalog {ident}") + except Exception: + logger.warning("Failed to drop catalog %s", ident) return False diff --git a/clients/client-python/gravitino/dto/dto_converters.py b/clients/client-python/gravitino/dto/dto_converters.py index 515c0c17652..801f9b07d5d 100644 --- a/clients/client-python/gravitino/dto/dto_converters.py +++ b/clients/client-python/gravitino/dto/dto_converters.py @@ -20,14 +20,14 @@ def to_metalake_update_request(change: MetalakeChange) -> object: # Assuming MetalakeUpdateRequest has similar nested class structure for requests if isinstance(change, MetalakeChange.RenameMetalake): return MetalakeUpdateRequest.RenameMetalakeRequest(change.new_name()) - elif isinstance(change, MetalakeChange.UpdateMetalakeComment): + if isinstance(change, MetalakeChange.UpdateMetalakeComment): return MetalakeUpdateRequest.UpdateMetalakeCommentRequest(change.new_comment()) - elif isinstance(change, MetalakeChange.SetProperty): + if isinstance(change, MetalakeChange.SetProperty): return MetalakeUpdateRequest.SetMetalakePropertyRequest(change.property(), change.value()) - elif isinstance(change, MetalakeChange.RemoveProperty): + if isinstance(change, MetalakeChange.RemoveProperty): return MetalakeUpdateRequest.RemoveMetalakePropertyRequest(change.property()) - else: - raise ValueError(f"Unknown change type: {type(change).__name__}") + + raise ValueError(f"Unknown change type: {type(change).__name__}") @staticmethod def to_catalog(catalog: CatalogDTO, client: HTTPClient): @@ -39,18 +39,20 @@ def to_catalog(catalog: CatalogDTO, client: HTTPClient): properties=catalog.properties(), audit=catalog.audit_info(), rest_client=client) - else: - raise NotImplementedError("Unsupported catalog type: " + str(catalog.type())) + + raise NotImplementedError("Unsupported catalog type: " + str(catalog.type())) @staticmethod def to_catalog_update_request(change: CatalogChange): if isinstance(change, CatalogChange.RenameCatalog): return CatalogUpdateRequest.RenameCatalogRequest(change.new_name) - elif isinstance(change, CatalogChange.UpdateCatalogComment): + if isinstance(change, CatalogChange.UpdateCatalogComment): return CatalogUpdateRequest.UpdateCatalogCommentRequest(change.new_comment) - elif isinstance(change, CatalogChange.SetProperty): + if isinstance(change, CatalogChange.SetProperty): + # TODO + # pylint: disable=too-many-function-args return CatalogUpdateRequest.SetCatalogPropertyRequest(change.property(), change.value()) - elif isinstance(change, CatalogChange.RemoveProperty): + if isinstance(change, CatalogChange.RemoveProperty): return CatalogUpdateRequest.RemoveCatalogPropertyRequest(change._property) - else: - raise ValueError(f"Unknown change type: {type(change).__name__}") + + raise ValueError(f"Unknown change type: {type(change).__name__}") diff --git a/clients/client-python/gravitino/dto/requests/fileset_updates_request.py b/clients/client-python/gravitino/dto/requests/fileset_updates_request.py index 2552c1869f1..14c1766dc51 100644 --- a/clients/client-python/gravitino/dto/requests/fileset_updates_request.py +++ b/clients/client-python/gravitino/dto/requests/fileset_updates_request.py @@ -3,7 +3,7 @@ This software is licensed under the Apache License version 2. """ from dataclasses import dataclass, field -from typing import Optional, List +from typing import List from dataclasses_json import config diff --git a/clients/client-python/gravitino/dto/responses/entity_list_response.py b/clients/client-python/gravitino/dto/responses/entity_list_response.py index 91beae6d3b9..657df59e0a9 100644 --- a/clients/client-python/gravitino/dto/responses/entity_list_response.py +++ b/clients/client-python/gravitino/dto/responses/entity_list_response.py @@ -3,7 +3,7 @@ This software is licensed under the Apache License version 2. """ from dataclasses import dataclass, field -from typing import Optional, List +from typing import List from dataclasses_json import config diff --git a/clients/client-python/gravitino/dto/responses/schema_response.py b/clients/client-python/gravitino/dto/responses/schema_response.py index 04e497feed9..501b34e2390 100644 --- a/clients/client-python/gravitino/dto/responses/schema_response.py +++ b/clients/client-python/gravitino/dto/responses/schema_response.py @@ -14,7 +14,9 @@ class SchemaResponse(BaseResponse, DataClassJsonMixin): """Represents a response for a schema.""" _schema: SchemaDTO = field(metadata=config(field_name='schema')) - + + # TODO + # pylint: disable=arguments-differ def schema(self) -> SchemaDTO: return self._schema diff --git a/clients/client-python/gravitino/exceptions/gravitino_runtime_exception.py b/clients/client-python/gravitino/exceptions/gravitino_runtime_exception.py index 8469535d82b..520b6a7ec5a 100644 --- a/clients/client-python/gravitino/exceptions/gravitino_runtime_exception.py +++ b/clients/client-python/gravitino/exceptions/gravitino_runtime_exception.py @@ -9,4 +9,3 @@ class GravitinoRuntimeException(RuntimeError): def __init__(self, message, *args): super().__init__(message.format(*args)) - diff --git a/clients/client-python/gravitino/exceptions/illegal_name_identifier_exception.py b/clients/client-python/gravitino/exceptions/illegal_name_identifier_exception.py index fff6e5715df..f8b5970ba1a 100644 --- a/clients/client-python/gravitino/exceptions/illegal_name_identifier_exception.py +++ b/clients/client-python/gravitino/exceptions/illegal_name_identifier_exception.py @@ -7,8 +7,8 @@ class IllegalNameIdentifierException(Exception): """An exception thrown when a name identifier is invalid.""" - def __init__(self, message=None, *args): + def __init__(self, message=None): if message: - super().__init__(message.format(*args)) + super().__init__(message) else: super().__init__() diff --git a/clients/client-python/gravitino/exceptions/illegal_namespace_exception.py b/clients/client-python/gravitino/exceptions/illegal_namespace_exception.py index b3ec0c26998..eecfdcfb5c5 100644 --- a/clients/client-python/gravitino/exceptions/illegal_namespace_exception.py +++ b/clients/client-python/gravitino/exceptions/illegal_namespace_exception.py @@ -7,8 +7,8 @@ class IllegalNamespaceException(Exception): """An exception thrown when a namespace is invalid.""" - def __init__(self, message=None, *args): + def __init__(self, message=None): if message: - super().__init__(message.format(*args)) + super().__init__(message) else: super().__init__() diff --git a/clients/client-python/gravitino/exceptions/no_such_metalake_exception.py b/clients/client-python/gravitino/exceptions/no_such_metalake_exception.py index ed3dc536275..7e540168374 100644 --- a/clients/client-python/gravitino/exceptions/no_such_metalake_exception.py +++ b/clients/client-python/gravitino/exceptions/no_such_metalake_exception.py @@ -7,6 +7,3 @@ class NoSuchMetalakeException(NotFoundException): """An exception thrown when a metalake is not found.""" - - def __init__(self, message, *args): - super().__init__(message, *args) diff --git a/clients/client-python/gravitino/exceptions/not_found_exception.py b/clients/client-python/gravitino/exceptions/not_found_exception.py index 87f3caca521..3a51576571b 100644 --- a/clients/client-python/gravitino/exceptions/not_found_exception.py +++ b/clients/client-python/gravitino/exceptions/not_found_exception.py @@ -7,6 +7,3 @@ class NotFoundException(GravitinoRuntimeException): """Base class for all exceptions thrown when a resource is not found.""" - - def __init__(self, message, *args): - super().__init__(message, *args) diff --git a/clients/client-python/gravitino/name_identifier.py b/clients/client-python/gravitino/name_identifier.py index f18805f5b15..987f5b1ac68 100644 --- a/clients/client-python/gravitino/name_identifier.py +++ b/clients/client-python/gravitino/name_identifier.py @@ -263,8 +263,7 @@ def __hash__(self): def __str__(self): if self.has_namespace(): return str(self._namespace) + "." + self._name - else: - return self._name + return self._name @staticmethod def check(condition, message, *args): diff --git a/clients/client-python/gravitino/utils/http_client.py b/clients/client-python/gravitino/utils/http_client.py index b5219d52e92..588c72ccfff 100644 --- a/clients/client-python/gravitino/utils/http_client.py +++ b/clients/client-python/gravitino/utils/http_client.py @@ -65,8 +65,6 @@ def headers(self): def json(self): if self.body: return _json.loads(self.body.decode("utf-8")) - else: - return None class HTTPClient: @@ -87,12 +85,12 @@ def _build_url(self, endpoint=None, params=None): url = self.host if endpoint: - url = "{}/{}".format(url.rstrip("/"), endpoint.lstrip("/")) + url = f"{url.rstrip('/')}/{endpoint.lstrip('/')}" if params: params = {k: v for k, v in params.items() if v is not None} url_values = urlencode(sorted(params.items()), True) - url = "{}?{}".format(url, url_values) + url = f"{url}?{url_values}" return url @@ -117,8 +115,7 @@ def _make_request(self, opener, request, timeout=None): return opener.open(request, timeout=timeout) except HTTPError as err: exc = handle_error(err) - exc.__cause__ = None - raise exc + raise exc from None def _request( self, method, endpoint, params=None, json=None, headers=None, timeout=None @@ -159,7 +156,7 @@ def put(self, endpoint, json=None, **kwargs): return self._request("put", endpoint, json=json, **kwargs) def close(self): - self._request("close") + self._request("close", "/") def unpack(path: str): diff --git a/clients/client-python/pylintrc b/clients/client-python/pylintrc new file mode 100644 index 00000000000..ec5ace82c43 --- /dev/null +++ b/clients/client-python/pylintrc @@ -0,0 +1,55 @@ +# Copyright 2024 Datastrato Pvt Ltd. +# This software is licensed under the Apache License version 2. + +[MESSAGES CONTROL] + +# Only show warnings with the listed confidence levels. Leave empty to show +# all. Valid levels: HIGH, INFERENCE, INFERENCE_FAILURE, UNDEFINED. +confidence= + +# Enable the message, report, category or checker with the given id(s). +# enable=logging + +# Disable the message, report, category or checker with the given id(s). +disable=missing-class-docstring, + missing-function-docstring, + missing-module-docstring, + global-variable-not-assigned, + too-many-public-methods, + too-few-public-methods, + unused-argument, + no-else-break, + no-member, + fixme, + unnecessary-pass, + missing-timeout, #TODO-fix + redefined-builtin, #TODO-fix + broad-exception-caught, #TODO-fix + duplicate-code, #TODO-fix + method-hidden, #TODO-fix + no-name-in-module, #TODO-fix + invalid-field-call, #TODO-fix + protected-access, #TODO-fix + too-many-arguments, #TODO-fix + inconsistent-return-statements, #TODO-fix + line-too-long, #TODO-config + invalid-name, #TODO-config + trailing-whitespace, #TODO-config + missing-final-newline #TODO-config + +[LOGGING] + +# The type of string formatting that logging methods do. `old` means using % +# formatting, `new` is for `{}` formatting. +logging-format-style=old + +# Logging modules to check that the string format arguments are in logging +# function parameter format. +logging-modules=logging + +[TYPECHECK] + +# List of members which are set dynamically and missed by pylint inference +# system, and so shouldn't trigger E1101 when accessed. Python regular +# expressions are accepted. +generated-members=gravitino.utils.http_client.Response diff --git a/clients/client-python/requirements-dev.txt b/clients/client-python/requirements-dev.txt index 16f503518f9..b92555189c1 100644 --- a/clients/client-python/requirements-dev.txt +++ b/clients/client-python/requirements-dev.txt @@ -1,4 +1,5 @@ # Copyright 2024 Datastrato Pvt Ltd. # This software is licensed under the Apache License version 2. requests -dataclasses-json \ No newline at end of file +dataclasses-json +pylint \ No newline at end of file diff --git a/clients/client-python/tests/integration/integration_test_env.py b/clients/client-python/tests/integration/integration_test_env.py index ef4d7bbdaf0..5bdb50a7c26 100644 --- a/clients/client-python/tests/integration/integration_test_env.py +++ b/clients/client-python/tests/integration/integration_test_env.py @@ -7,6 +7,7 @@ import unittest import subprocess import time +import sys import requests @@ -19,7 +20,7 @@ def get_gravitino_server_version(): response.raise_for_status() # raise an exception for bad status codes response.close() return True - except requests.exceptions.RequestException as e: + except requests.exceptions.RequestException: logger.warning("Failed to access the Gravitino server") return False @@ -45,7 +46,7 @@ class IntegrationTestEnv(unittest.TestCase): @classmethod def setUpClass(cls): if os.environ.get('START_EXTERNAL_GRAVITINO') is not None: - """Maybe Gravitino server already startup by Gradle test command or developer manual startup.""" + # Maybe Gravitino server already startup by Gradle test command or developer manual startup. if not check_gravitino_server_status(): logger.error("ERROR: Can't find online Gravitino server!") return @@ -53,19 +54,19 @@ def setUpClass(cls): GravitinoHome = os.environ.get('GRAVITINO_HOME') if GravitinoHome is None: logger.error('Gravitino Python client integration test must configure `GRAVITINO_HOME`') - quit(0) + sys.exit(0) cls.gravitino_startup_script = os.path.join(GravitinoHome, 'bin/gravitino.sh') if not os.path.exists(cls.gravitino_startup_script): logger.error("Can't find Gravitino startup script: %s, " "Please execute `./gradlew compileDistribution -x test` in the Gravitino project root " "directory.", cls.gravitino_startup_script) - quit(0) + sys.exit(0) logger.info("Starting integration test environment...") # Start Gravitino Server - result = subprocess.run([cls.gravitino_startup_script, 'start'], capture_output=True, text=True) + result = subprocess.run([cls.gravitino_startup_script, 'start'], capture_output=True, text=True, check=False) if result.stdout: logger.info("stdout: %s", result.stdout) if result.stderr: @@ -73,7 +74,7 @@ def setUpClass(cls): if not check_gravitino_server_status(): logger.error("ERROR: Can't start Gravitino server!") - quit(0) + sys.exit(0) @classmethod def tearDownClass(cls): @@ -81,7 +82,7 @@ def tearDownClass(cls): return logger.info("Stop integration test environment...") - result = subprocess.run([cls.gravitino_startup_script, 'stop'], capture_output=True, text=True) + result = subprocess.run([cls.gravitino_startup_script, 'stop'], capture_output=True, text=True, check=False) if result.stdout: logger.info("stdout: %s", result.stdout) if result.stderr: diff --git a/clients/client-python/tests/integration/test_metalake.py b/clients/client-python/tests/integration/test_metalake.py index 612245d460d..c7a80d068d4 100644 --- a/clients/client-python/tests/integration/test_metalake.py +++ b/clients/client-python/tests/integration/test_metalake.py @@ -85,8 +85,8 @@ def test_metalake_update_request_to_json(self): ) reqs = [DTOConverters.to_metalake_update_request(change) for change in changes] updates_request = MetalakeUpdatesRequest(reqs) - valid_json = (f'{{"updates": [{{"@type": "rename", "newName": "my_metalake_new"}}, ' - f'{{"@type": "updateComment", "newComment": "new metalake comment"}}]}}') + valid_json = ('{"updates": [{"@type": "rename", "newName": "my_metalake_new"}, ' + '{"@type": "updateComment", "newComment": "new metalake comment"}]}') self.assertEqual(updates_request.to_json(), valid_json) def test_from_json_metalake_response(self):