Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lift pylint ignore: too-many-positional-arguments #137

Open
bh2smith opened this issue Oct 8, 2024 · 5 comments
Open

Lift pylint ignore: too-many-positional-arguments #137

bh2smith opened this issue Oct 8, 2024 · 5 comments
Assignees
Labels
help wanted Extra attention is needed

Comments

@bh2smith
Copy link
Collaborator

bh2smith commented Oct 8, 2024

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

  1. 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.
  2. 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.
  3. 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:

  1. Refactor the method that is being flagged.
  2. Group the arguments into a dictionary or a class.
  3. 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

  1. 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,
})
  1. 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!

@bh2smith bh2smith added the help wanted Extra attention is needed label Oct 8, 2024
@apurvabanka
Copy link

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

@bh2smith
Copy link
Collaborator Author

Sure thing. Go for it.

@apurvabanka
Copy link

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
Copy link

@bh2smith How to handle params for create_table function?

def create_table(
self,
namespace: str,
table_name: str,
schema: List[Dict[str, str]],
description: str = "",
is_private: bool = False,
) -> CreateTableResult:

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

params={
"namespace": namespace,
"table_name": table_name,
"schema": schema,
"description": description,
"is_private": is_private,
},

Thoughts?

@apurvabanka
Copy link

PR raised for this issue - #139

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants