node,scylla_node,cluster,scylla_cluster: Add type hints for nodes and clusters #609
Conversation
cezarmoise
commented
Sep 12, 2024
cezarmoise
commented
- Add type hints for nodes and clusters to help debugging in dtest
- Fix linting / formatting issues
|
I will do a jenkins run to check for issues |
|
I would recommend add some linter that can identify those, I think ruff might be use to force adding type notation or |
Those tools check all function parameters. For example, with ruff I get that there are about 3000 untyped parameters or function returns. |
https://jenkins.scylladb.com/view/master/job/scylla-master/job/byo/job/dtest-byo/564/ |
cezarmoise
force-pushed
the
node-cluster-type-hints
branch
from
8d9a369 to
df0a17a
Compare
|
df0a17a rebased to solve conflicts |
There was a problem hiding this comment.
My only issue is that typing.* are deprecated since 3.9, because now you can use inbuilt types for typing instead, e.g. test: list[str] = []
See https://docs.python.org/3/library/typing.html#aliases-to-types-in-collections
Not sure whaat is our minimal working version, but 3.9 seems old & stable enough to do it the new way. @fruch WDYT?
| @@ -1125,7 +1127,7 @@ def get_sstablespath(self, output_file=None, datafiles=None, keyspace=None, tabl | |||
| return sstablefiles | |||
|
|
|||
| def run_sstablerepairedset(self, set_repaired=True, datafiles=None, keyspace=None, column_families=None): | |||
| cdir = self.get_install_dir() | |||
| cdir = self.get_install_dir() # noqa: F841 | |||
There was a problem hiding this comment.
Why do we have unused-variables here? (Applies to all occurrences)
There was a problem hiding this comment.
Cause we never linted this code.
This code path also probably isn't in use...
The minimum is 3.8, we need to support the same versions as the python-driver does, cause ccm is used in it's testing The typing.* isn't yet deprecated AFAIK, linters are suggesting not to use it. |
| @@ -246,14 +249,14 @@ def add(self, node, is_seed, data_center=None, rack=None): | |||
|
|
|||
| # nodes can be provided in multiple notations, determining the cluster topology: | |||
| # 1. int - specifying the number of nodes in a single DC, single RACK cluster | |||
| # 2. list[int] - specifying the number of nodes in a multi DC, single RACK per DC cluster | |||
| # 2. List[int] - specifying the number of nodes in a multi DC, single RACK per DC cluster | |||
There was a problem hiding this comment.
No real need to change those comments
| def new_node(self, i, auto_bootstrap=False, debug=False, initial_token=None, add_node=True, is_seed=True, data_center=None, rack=None): | ||
| ipformat = self.get_ipformat() | ||
| def new_node(self, i, auto_bootstrap=False, debug=False, initial_token=None, add_node=True, is_seed=True, data_center=None, rack=None) -> ScyllaNode: | ||
| ipformat = self.get_ipformat() # noqa: F841 |
There was a problem hiding this comment.
What is the point of introducing noqa ? If we don't introduce the linter as well ?
| @@ -365,7 +368,7 @@ def get_storage_interface(self, nodeid): | |||
| return (self.get_node_ip(nodeid), 7000) | |||
|
|
|||
| def get_node_jmx_port(self, nodeid): | |||
| return 7000 + nodeid * 100 + self.id; | |||
There was a problem hiding this comment.
Shame to get rid of ANSI C, isn't it ? 😜
| @@ -421,7 +424,7 @@ def remove_dir_with_retry(self, path): | |||
| try: | |||
| common.rmdirs(path) | |||
| removed = True | |||
| except: | |||
| except Exception: | |||
There was a problem hiding this comment.
Linter should be angry about this, isn't it ? too broad exception ?
I think in this case we can set a specific set of possible exceptions
