Lift pylint ignore: too-many-positional-arguments

bh2smith commented 1 month ago

This came up in #136 - Here are some AI suggestions:

The Pylint error R0917: Too many positional arguments (8/5) indicates that a function or method is being called with more than the recommended number of positional arguments (the default limit is 5). This is generally considered a code smell because it suggests that the function might be doing too much or its parameters could be better organized.

Solutions

Refactor to Use Keyword Arguments: If many of the arguments have a logical grouping or hierarchy, you can use keyword arguments to make the function call more readable and avoid using too many positional arguments.
Use a Data Structure (e.g., Dictionary or Class): Instead of passing many individual arguments, consider grouping related arguments into a dictionary or a custom class.
Use Default Values and Optional Parameters: Reduce the number of required positional arguments by assigning default values to some of them.

How to Address the Issue in Your Code

In your case, you can:

Refactor the method that is being flagged.
Group the arguments into a dictionary or a class.
Use keyword arguments instead of positional arguments where applicable.

Example Solution

Let’s assume the method some_function is causing the issue. Here’s how you can refactor it:

Original Method (Example)

def _get_result_page(
    self,
    job_id: str,
    limit: Optional[int] = None,
    offset: Optional[int] = None,
    columns: Optional[List[str]] = None,
    sample_count: Optional[int] = None,
    filters: Optional[str] = None,
    sort_by: Optional[List[str]] = None,
) -> ResultsResponse:
    """GET a page of results from Dune API for `job_id` (aka `execution_id`)"""
    # Method logic here...

If this is being called with 8 positional arguments like:

await self._get_result_page(job_id, limit, offset, columns, sample_count, filters, sort_by)

This will trigger the R0917 error.

Refactored Solution

Use a Dictionary to Group Parameters

def _get_result_page(
self,
job_id: str,
params: Optional[Dict[str, Any]] = None,
) -> ResultsResponse:
"""GET a page of results from Dune API for `job_id` (aka `execution_id`)"""
if params is None:
    params = {}
# Use params dictionary to access limit, offset, columns, etc.
limit = params.get("limit", None)
offset = params.get("offset", None)
columns = params.get("columns", None)
sample_count = params.get("sample_count", None)
filters = params.get("filters", None)
sort_by = params.get("sort_by", None)
# Method logic here...

Then call it like this:

await self._get_result_page(job_id, params={
"limit": limit,
"offset": offset,
"columns": columns,
"sample_count": sample_count,
"filters": filters,
"sort_by": sort_by,
})

Use a Named Tuple or Data Class

If you want more structure, use a named tuple or a data class:

from typing import NamedTuple

class ResultPageParams(NamedTuple):
    limit: Optional[int]
    offset: Optional[int]
    columns: Optional[List[str]]
    sample_count: Optional[int]
    filters: Optional[str]
    sort_by: Optional[List[str]]

def _get_result_page(
    self,
    job_id: str,
    params: ResultPageParams,
) -> ResultsResponse:
    """GET a page of results from Dune API for `job_id` (aka `execution_id`)"""
    # Use params.limit, params.offset, params.columns, etc.

Then call it like this:

params = ResultPageParams(limit, offset, columns, sample_count, filters, sort_by)
await self._get_result_page(job_id, params)

This way, you reduce the number of positional arguments passed to the function and increase the readability of your code.

Applying the Solution to Your Case

Given the specific function signature you shared, you can apply either of the two approaches above based on your preference and the specific codebase context. If you provide me with the exact method signature and context where the error is occurring, I can give a more targeted solution tailored to that code snippet!

apurvabanka commented 1 month ago

@bh2smith I would like to contribute. Can you assign me this issue?

bh2smith commented 1 month ago

Sure thing. Go for it.

apurvabanka commented 1 month ago

List of Functions that need to be revamped.

Module dune_client.client_async dune_client/client_async.py:232:4: R0917: Too many positional arguments (7/5) (too-many-positional-arguments) dune_client/client_async.py:266:4: R0917: Too many positional arguments (7/5) (too-many-positional-arguments) dune_client/client_async.py:356:4: R0917: Too many positional arguments (9/5) (too-many-positional-arguments) dune_client/client_async.py:391:4: R0917: Too many positional arguments (9/5) (too-many-positional-arguments) dune_client/client_async.py:426:4: R0917: Too many positional arguments (8/5) (too-many-positional-arguments) dune_client/client_async.py:463:4: R0917: Too many positional arguments (8/5) (too-many-positional-arguments) dune_client/client_async.py:512:4: R0917: Too many positional arguments (8/5) (too-many-positional-arguments) Module dune_client.api.execution dune_client/api/execution.py:75:4: R0917: Too many positional arguments (9/5) (too-many-positional-arguments) dune_client/api/execution.py:101:4: R0917: Too many positional arguments (8/5) (too-many-positional-arguments) Module dune_client.api.custom dune_client/api/custom.py:24:4: R0917: Too many positional arguments (9/5) (too-many-positional-arguments) Module dune_client.api.query dune_client/api/query.py:57:4: R0917: Too many positional arguments (7/5) (too-many-positional-arguments) Module dune_client.api.extensions dune_client/api/extensions.py:47:4: R0917: Too many positional arguments (10/5) (too-many-positional-arguments) dune_client/api/extensions.py:91:4: R0917: Too many positional arguments (9/5) (too-many-positional-arguments) dune_client/api/extensions.py:133:4: R0917: Too many positional arguments (9/5) (too-many-positional-arguments) dune_client/api/extensions.py:168:4: R0917: Too many positional arguments (8/5) (too-many-positional-arguments) dune_client/api/extensions.py:241:4: R0917: Too many positional arguments (7/5) (too-many-positional-arguments) dune_client/api/extensions.py:274:4: R0917: Too many positional arguments (7/5) (too-many-positional-arguments) dune_client/api/extensions.py:327:4: R0917: Too many positional arguments (8/5) (too-many-positional-arguments) Module dune_client.api.table dune_client/api/table.py:55:4: R0917: Too many positional arguments (6/5) (too-many-positional-arguments) ***** Module dune_client.api.base dune_client/api/base.py:33:4: R0917: Too many positional arguments (6/5) (too-many-positional-arguments) dune_client/api/base.py:90:4: R0917: Too many positional arguments (9/5) (too-many-positional-arguments)

apurvabanka commented 1 month ago

@bh2smith How to handle params for create_table function?

https://github.com/duneanalytics/dune-client/blob/7d7d14ea950845fc9d715dada33d29c574c237ad/dune_client/api/table.py#L55-L62

We can create a dict with all the params and pass it in line 75.

https://github.com/duneanalytics/dune-client/blob/7d7d14ea950845fc9d715dada33d29c574c237ad/dune_client/api/table.py#L75-L81

Thoughts?

apurvabanka commented 1 month ago

PR raised for this issue - #139

duneanalytics / dune-client