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

Custom scalars using numpy arrays fail is_nullish() test #59

Closed
thomascobb opened this issue Sep 13, 2019 · 3 comments · Fixed by #60
Closed

Custom scalars using numpy arrays fail is_nullish() test #59

thomascobb opened this issue Sep 13, 2019 · 3 comments · Fixed by #60

Comments

@thomascobb
Copy link
Contributor

I have a custom scalar type for a numpy array:

def serialize_ndarray(value):
    return dict(
        numberType=value.dtype.name.upper(),
        base64=base64.b64encode(value).decode()
    )

ndarray_type = GraphQLScalarType("NDArray", serialize=serialize_ndarray)

is_nullish() does an inequality test on values to work out if they are NaN:

def is_nullish(value: Any) -> bool:
    """Return true if a value is null, undefined, or NaN."""
    return value is None or value is INVALID or value != value

Unfortunately, numpy arrays break this because they override __eq__ to return a pointwise equality array rather than a boolean.

The comment for is_nullish() suggests that value != value is checking for NaN. If this is the case we could replace it with:

def is_nan(value):
    """Return true if a value is NaN"""
    try:
        return math.isnan(value)
    except TypeError:
        return False

def is_nullish(value: Any) -> bool:
    """Return true if a value is null, undefined, or NaN."""
    return value is None or value is INVALID or is_nan(value)

Would this be acceptable? I can provide a PR if it is.

@Cito
Copy link
Member

Cito commented Sep 13, 2019

Right, this value != value is a bit obscure - it has been taken over 1:1 from JavaScript. Not sure why they test it that way, probably because isNaN() was only added to the language in ES 6.

I'm not against changing this and using math.isnan instead. However, I'd rather avoid creating an exception and do something like ... or (isinstance(value, float) and isnan(value)) instead.

We should also add a test that is_nullish(value) is False for some custom object with value != value , that would simulate the numpy array. Feel free to send a PR, otherwise I'll care about this.

@thomascobb
Copy link
Contributor Author

Ok, I think I'm happy with the PR, let me know if any changes are required for it

@Cito Cito closed this as completed in #60 Sep 13, 2019
@Cito
Copy link
Member

Cito commented Sep 13, 2019

Nice, thank you! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants