[#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 <[email protected]>

clients/client-python/.gitignore

@@ -5,5 +5,7 @@
 **/__pycache__/**
 gravitino.egg-info
 vevn
+venv
 .vevn
+.venv
 .idea

clients/client-python/build.gradle.kts

@@ -38,14 +38,19 @@ 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) {
       doFirst {
         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) {

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

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

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)

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

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__}")

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
 

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
 

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
 

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))
-

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__()

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__()

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)

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)

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):

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):

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

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
+dataclasses-json
+pylint