Skip to content

PR: Rename signatures module to array_api #430

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

Merged
merged 8 commits into from
May 23, 2022

Conversation

steff456
Copy link
Member

@steff456 steff456 commented May 11, 2022

This PR,

  • Rename the signatures module to array_api
  • Add imports in the __init__.py to provide a flat namespace of all the core modules (extensions are not included)

With these changes I'm still not able to use the flat namespace name in the references when rendering with sphinx, but it should work if you are importing the module from your documentation @leofang. Please let me know if it works!

Fixes #428

@steff456 steff456 added the RFC Request for comments. Feature requests and proposed changes. label May 11, 2022
@steff456 steff456 added this to the v2022 milestone May 11, 2022
@steff456 steff456 requested review from asmeurer, kgryte and leofang May 11, 2022 16:08
@steff456 steff456 self-assigned this May 11, 2022
@leofang
Copy link
Contributor

leofang commented May 11, 2022

Thanks, Stephannie! That's quick. Let me try the patch locally later tonight.

@leofang
Copy link
Contributor

leofang commented May 12, 2022

Hi @steff456 and all, I hope you don't mind, I just pushed 9a25cdf to make sure the flattened namespace is accessible by intersphinx. The fact that with this change our own documentation is also using the flattened namespace means that we're testing it ourselves before publishing. A command line check is this:

$ python -m sphinx.ext.intersphinx _build/html/objects.inv

It should list what functions, attributes, etc are accessible to intersphinx and their corresponding hyperlinks.

@leofang
Copy link
Contributor

leofang commented May 12, 2022

Not sure why but the CI hits a permission issue...?

@leofang
Copy link
Contributor

leofang commented May 12, 2022

Not sure why but the CI hits a permission issue...?

Ah OK, I also hit this issue before. It's because this GH action dislikes PRs from forking branches... I think this is ready for review.

@asmeurer
Copy link
Member

This can also be useful for verifying this https://github.com/bskinn/sphobjinv

@leofang
Copy link
Contributor

leofang commented May 15, 2022

@asmeurer @honno Will the changes here impact the test suite?

@asmeurer
Copy link
Member

Yeah they will, but that shouldn't block the merging here. We pin the array-api repo with a submodule so we can update it independently.

Copy link
Contributor

@leofang leofang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Giving my 👍 despite I also worked on this PR 🙂

Copy link
Contributor

@kgryte kgryte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks, @steff456, @leofang, @honno. Let's get this in, and try it out.

I updated the PR to follow @honno's suggestion of importing data_types as dtype. Hence, @leofang you'll want to document against dtype.__eq__.

@kgryte kgryte merged commit fc77b0a into data-apis:main May 23, 2022
@kgryte kgryte deleted the rename-sig-module branch May 23, 2022 00:03
@leofang
Copy link
Contributor

leofang commented May 23, 2022

Thanks, everyone!

@leofang
Copy link
Contributor

leofang commented May 23, 2022

Continue the discussion back in the original issue #428.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request for comments. Feature requests and proposed changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Intersphinx cross-referencing does not work naively
5 participants