Too many positional arguments #139

apurvabanka · 2024-10-21T00:29:00Z

No description provided.

bh2smith

So everything looks good here so far. It seems that there is some duplication (i.e. x = params.get("x", None) appears multiple times) that we could probably avoid with a dedicated parameter parsing function.

On another note, this improvement is going to break the current interface and probably warrants a full version bump (i.e. v2). Since this is the first breaking change introduced and its not overly substantial. I would propose creating a v2 branch that we can merge this into and it would be introduced when the project is preparing for v2. This way, you would still become a project contributor although the changes may not be reflected for quite a some time.

I wonder if the Dune team has any thoughts on how we should approach this... @msf?.

In the meantime, I will create a v2 branch and update this PR to point there.

apurvabanka · 2024-10-22T04:49:01Z

@bh2smith I have added a comment in the issue linked to this PR. Can you respond with your suggestions?

#137

apurvabanka · 2024-10-22T18:19:42Z

@bh2smith Updated the PR with the changes and test cases. Please proceed with the review.

apurvabanka · 2024-10-27T16:50:45Z

@bh2smith Can you review this PR? I have added all the changes and test cases as well.

bh2smith · 2024-10-27T20:59:08Z

Hey sorry its been so long. I was kinda waiting on @msf here but I guess we will just go with my suggestion of creating a v2 branch and merging this there.

I do have one, somewhat large, ask about how we approach this.

Instead of using a dictionary for the params I would insist that we use a data class for two reasons.

Accessing entries of a dictionary by key is annoying params["something"] vs params.something
Having a data class, gives developers some inference on what is expected to go there directly in their IDE without having to go to the docs to try and remember.

So I would request that you replace the occurrences of params: Optional[Dict[str, Any]] = None with params: Optional[ResultPageParams] = None

where:

from dataclasses import dataclass
from typing import Optional, List

@dataclass
class ResultPageParams:
    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

and as for your question

How to handle params for create_table function?

I would suggest doing the same thing. That is create a data class representing some the optional arguments (i.e. those with defaults set {description & is_private}).

Alternatively, we could change pylint to allow 6 parameters instead of 5.

Sorry again for taking so long to get back. I hope the suggested change isn't too involved, but I think it will result in a much better end product. We may also be able to sneak this feature into v1, by taking on the new dataclass as an argument and warning users that the other parameters will deprecated in v2.

bh2smith · 2024-10-27T21:04:07Z

dune_client/api/base.py

        allow_partial_results: str = "true",
+        params: Optional[Dict[str, Any]] = None,


A good way to start here would be, create the proposed data class and put it here as optional.

See how the function needs to be changed because of the replacement...

Actually, I just noticed this is _build_parameters... This will likely be replaces with a method like to_dict on the class itself. The data class can implement both to_dict and from_dict (this might come for free with the @DataClass decorator although I can't remember).

Nit: It looks like you forgot to include allow_partial_results here, but I really think this _build_parameters function will no longer be necessary once this class is in place.

bh2smith · 2024-10-27T21:05:16Z

dune_client/api/custom.py

-        params = self._build_parameters(
-            columns=columns,
-            sample_count=sample_count,
-            filters=filters,
-            sort_by=sort_by,
-            limit=limit,
-            offset=offset,
+        if params is None:
+            params = {}
+        limit = params.get("limit", None)
+        offset = params.get("offset", None)
+        columns = params.get("columns", None)
+        sample_count = params.get("sample_counts", None)
+        filters = params.get("filters", None)
+        sort_by = params.get("sort_by", None)
+
+        build_params = self._build_parameters(
+            params={
+                "columns": columns,
+                "sample_count": sample_count,
+                "filters": filters,
+                "sort_by": sort_by,
+                "limit": limit,
+                "offset": offset,
+            }
        )


All of this should become unnecessary and all we will need to do is params.to_dict() wherever it gets passed into a request.

bh2smith · 2024-10-27T21:05:36Z

dune_client/api/execution.py

-        sample_count: Optional[int] = None,
-        filters: Optional[str] = None,
-        sort_by: Optional[List[str]] = None,
+        params: Optional[GetExecutionResultsParams] = None,


Hey, ya you did it here.

bh2smith · 2024-10-27T21:07:08Z

dune_client/api/execution.py

+        build_params = None
+        if params is not None:
+            build_params = self._build_parameters(
+                params={
+                    "columns": params.columns,
+                    "sample_count": params.sample_count,
+                    "filters": params.filters,
+                    "sort_by": params.sort_by,
+                    "limit": params.limit,
+                    "offset": params.offset,
+                }
+            )


I think this is unnecessary. and can either be passed to the next function as is. It should only be at the very last step it needs to be transformed to a dict.

bh2smith · 2024-10-27T21:07:53Z

dune_client/api/extensions.py

+        if params is None:
+            params = {}
+        performance = params.get("performance", None)
+        batch_size = params.get("batch_size", 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)


There is way too much redundant code here. All of this appears too many times over.

bh2smith · 2024-10-27T21:09:29Z

dune_client/client_async.py

@@ -40,6 +40,44 @@
 from dune_client.query import QueryBase, parse_query_object_or_id


+class GetResultParams(NamedTuple):


Ok so I see you have created all these classes, but its unclear where they are being used. Since there is so much dictionary manipulation happening above... Something seems wrong.

Should I add a doc string for their usage? I have re-used these classes as parameters for multiple functions.

apurvabanka · 2024-10-27T21:44:50Z

Hey sorry its been so long. I was kinda waiting on @msf here but I guess we will just go with my suggestion of creating a v2 branch and merging this there.

I do have one, somewhat large, ask about how we approach this.

Instead of using a dictionary for the params I would insist that we use a data class for two reasons.

Accessing entries of a dictionary by key is annoying params["something"] vs params.something

Having a data class, gives developers some inference on what is expected to go there directly in their IDE without having to go to the docs to try and remember.

So I would request that you replace the occurrences of params: Optional[Dict[str, Any]] = None with params: Optional[ResultPageParams] = None

where:
from dataclasses import dataclass
from typing import Optional, List

@dataclass
class ResultPageParams:
    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
and as for your question

How to handle params for create_table function?

I would suggest doing the same thing. That is create a data class representing some the optional arguments (i.e. those with defaults set {description & is_private}).

Alternatively, we could change pylint to allow 6 parameters instead of 5.

Sorry again for taking so long to get back. I hope the suggested change isn't too involved, but I think it will result in a much better end product. We may also be able to sneak this feature into v1, by taking on the new dataclass as an argument and warning users that the other parameters will deprecated in v2.

I do agree using classes is a better way to implement params. Will in-corporate those changes in the code.

apurvabanka added 3 commits October 20, 2024 20:24

.venv in gitignore

a8780ff

removing too-many-positional-arguments

def7152

changing function params

2afbf0e

bh2smith approved these changes Oct 21, 2024

View reviewed changes

bh2smith changed the base branch from main to v2 October 21, 2024 11:11

apurvabanka added 2 commits October 22, 2024 02:47

Implemented classes for common params

b07157d

test cases updated

9d59e08

apurvabanka marked this pull request as ready for review October 22, 2024 18:15

apurvabanka requested a review from bh2smith October 22, 2024 18:18

apurvabanka mentioned this pull request Oct 22, 2024

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

Open

bh2smith reviewed Oct 27, 2024

View reviewed changes

apurvabanka added 4 commits November 7, 2024 02:01

requested changes

c168c60

removing build params

a919f61

too-many-positional-arguments

a88310f

typo

fd32e14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Too many positional arguments #139

Too many positional arguments #139

apurvabanka commented Oct 21, 2024

bh2smith left a comment

apurvabanka commented Oct 22, 2024 •

edited

Loading

apurvabanka commented Oct 22, 2024

apurvabanka commented Oct 27, 2024

bh2smith commented Oct 27, 2024

bh2smith Oct 27, 2024

bh2smith Oct 27, 2024

bh2smith Oct 27, 2024

bh2smith Oct 27, 2024

bh2smith Oct 27, 2024

bh2smith Oct 27, 2024

apurvabanka Oct 27, 2024

apurvabanka commented Oct 27, 2024

		allow_partial_results: str = "true",
		params: Optional[Dict[str, Any]] = None,

		@@ -40,6 +40,44 @@
		from dune_client.query import QueryBase, parse_query_object_or_id


		class GetResultParams(NamedTuple):

Too many positional arguments #139

Are you sure you want to change the base?

Too many positional arguments #139

Conversation

apurvabanka commented Oct 21, 2024

bh2smith left a comment

Choose a reason for hiding this comment

apurvabanka commented Oct 22, 2024 • edited Loading

apurvabanka commented Oct 22, 2024

apurvabanka commented Oct 27, 2024

bh2smith commented Oct 27, 2024

bh2smith Oct 27, 2024

Choose a reason for hiding this comment

bh2smith Oct 27, 2024

Choose a reason for hiding this comment

bh2smith Oct 27, 2024

Choose a reason for hiding this comment

bh2smith Oct 27, 2024

Choose a reason for hiding this comment

bh2smith Oct 27, 2024

Choose a reason for hiding this comment

bh2smith Oct 27, 2024

Choose a reason for hiding this comment

apurvabanka Oct 27, 2024

Choose a reason for hiding this comment

apurvabanka commented Oct 27, 2024

apurvabanka commented Oct 22, 2024 •

edited

Loading