Too many positional arguments

[apurvabanka]

[apurvabanka]

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

#137

[apurvabanka]

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

[apurvabanka]

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

[bh2smith]

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.

1. Accessing entries of a dictionary by key is annoying params["something"] vs params.something
2. 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.

[apurvabanka]

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.

1. Accessing entries of a dictionary by key is annoying params["something"] vs params.something
2. 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.

[msf]

sorry I took too long to check on this.

This is a breaking change, so we should have a PR with a new major version bump and the new API changes should be stacked ontop of that.

if the main benefit of the version bump is just to remove linting warnings, its a lot of work for (just) a cosmetic change.

[bh2smith]

sorry I took too long to check on this.

This is a breaking change, so we should have a PR with a new major version bump and the new API changes should be stacked ontop of that.

if the main benefit of the version bump is just to remove linting warnings, its a lot of work for (just) a cosmetic change.

Yes, one thing to propose here is to merge this into a "beta" branch (actually it has already been directed at a v2 branch). Once v2 starts development this would be included as the first commit there. I suspect it will be quite some time before this package begins v2 development.