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

Fix type annotations and checks in functions.py #8903

Merged
merged 2 commits into from
Jun 12, 2020

Conversation

nawatts
Copy link
Contributor

@nawatts nawatts commented Jun 1, 2020

hail/python/hail/expr/functions.py defines functions str, int, etc. There are a few uses of str or int in functions.py that refer to hail.expr.functions.str and hail.expr.functions.int where they are intended to refer to builtins.str or builtins.int.

Empty collection constructors are currently documented as accepting either a Hail type object or a hail.expr.functions.str instead of a builtins.str for the type name.
https://hail.is/docs/0.2/functions/constructors.html#hail.expr.functions.empty_array

In the case of shuffle, the type check as well as the type annotation is incorrect. Currently, attempting to call hl.shuffle with a seed argument throws an exception TypeError: int() missing 1 required positional argument: 'x'.


There is also a more widespread but much less impactful issue with the docstrings in functions.py. Sphinx interprets :obj:`str` in a docstring in functions.py to refer to hail.expr.functions.str, so the documentation for many parameters of builtin types links to hail.expr.functions functions. For example, str in the description of the reference_genome parameter for hl.locus links to hl.expr.functions.str, though that parameter is actually of type builtin.str.
https://hail.is/docs/0.2/functions/genetics.html#hail.expr.functions.locus

@danking
Copy link
Contributor

danking commented Jun 1, 2020

CI doesn't recognize your GitHub account, I'm looking into why.

@danking
Copy link
Contributor

danking commented Jun 1, 2020

Should be fixed shortly. Apologies.

@nawatts
Copy link
Contributor Author

nawatts commented Jun 1, 2020

Thanks @danking. And no worries, I was mainly interested in being able to see test output vs having tests automatically start.

@danking danking merged commit 4a319c0 into hail-is:master Jun 12, 2020
@nawatts nawatts deleted the expr-functions-types branch June 12, 2020 17:36
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 this pull request may close these issues.

None yet

3 participants