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

Better signature tests for required arguments #281

Open
asmeurer opened this issue Jul 9, 2024 · 0 comments
Open

Better signature tests for required arguments #281

asmeurer opened this issue Jul 9, 2024 · 0 comments
Labels
medium priority Medium priority issue

Comments

@asmeurer
Copy link
Member

asmeurer commented Jul 9, 2024

This signature tests do not properly check for cases where a function has more required positional arguments than it should. Such a case should be considered noncompliant, since user code written against the standard signature would not function. For example, if the standard has a function

def f(x, y=None):
    ...

then a function

def f(x, y):
    ...

should fail the signature test, because code like f(a) should work according to the standard but it wouldn't in the implementation.

Practically speaking, this would help me catch bugs with the xp signature logic in array-api-compat (see https://data-apis.org/array-api-compat/dev/implementation-notes.html). For instance, I accidentally wrote

def clip(
    x: ndarray,
    /,
	xp,
    min: Optional[Union[int, float, ndarray]] = None,
    max: Optional[Union[int, float, ndarray]] = None,
) -> ndarray:

but it should have been

def clip(
    x: ndarray,
    /,
    min: Optional[Union[int, float, ndarray]] = None,
    max: Optional[Union[int, float, ndarray]] = None,
    *,
    xp,
) -> ndarray:

The former works if you pass min and max as keywords but not if you pass them positionally (array-api-compat has a decorator that automatically injects the xp keyword argument into the final signature).

Additionally, some helpers to test optional positional arguments as both positional and keyword in the test itself would be useful. Right now we have specified_kwargs() but it doesn't allow passing the arguments as positional.

@asmeurer asmeurer added the medium priority Medium priority issue label Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium priority Medium priority issue
Projects
None yet
Development

No branches or pull requests

1 participant