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

Add tests for function arguments #1303

Closed
alifeee opened this issue Sep 28, 2023 · 4 comments · Fixed by #1296
Closed

Add tests for function arguments #1303

alifeee opened this issue Sep 28, 2023 · 4 comments · Fixed by #1296
Assignees
Milestone

Comments

@alifeee
Copy link
Collaborator

alifeee commented Sep 28, 2023

Some functions are aliases of other functions. For example:

  • get_all_values and get_values
  • get_all_records and get_records

Over time, we may add kwargs and args to the original functions, and forget to add these to the aliases. We should add some simple tests to the test files that verify that the function arguments/kwargs are the same.

This issue alread arose once with get vs get_values.

@alifeee alifeee self-assigned this Sep 28, 2023
@alifeee alifeee added this to the 5.12 milestone Sep 28, 2023
@alifeee
Copy link
Collaborator Author

alifeee commented Sep 28, 2023

I don't know that this is possible for functions that use the @accepted_kwargs decorator

This may be easier in v6.0.0. Arguments and kwargs should be explicitly set, and the length can be obtained via the signature. See this SO post:

https://stackoverflow.com/a/41188411

from inspect import signature

def someMethod(self, arg1, kwarg1=None):
    pass

sig = signature(someMethod)

print(len(params))  # 3

@alifeee alifeee modified the milestones: 5.12, 6.0.0 Sep 28, 2023
@lavigne958
Copy link
Collaborator

The functions/methods using accepted_kwargs will be hard to update. We introduced a new special value to allow arguments with no value. Apart from that we can't do much.

The target is in 6.0.0 we removed the accepted_kwargs decorator so the truth is in the arguments themselves and their value too. And we will try to reduce the number of occurrences of **kwargs to make it explicit.

We can put it as a target:

  • remove all ocurences of **kwargs

What do you think?

@alifeee
Copy link
Collaborator Author

alifeee commented Sep 29, 2023

I agree, we should not have

def func(**kwargs):
...

and we should have

def func(kw_arg1=True, kw_arg2=False, ...)

I would like to add a simple test that get and get_values and get_all_values keep the same number of kwargs, for example (if/when we add new formatting options, they should be available to all functions)

This is ultimately low priority.

@alifeee
Copy link
Collaborator Author

alifeee commented Nov 6, 2023

closed by #1296

@alifeee alifeee closed this as completed Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants