Skip to content

PR: Transform sorting functions from md to rst file #333

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 10 commits into from
Dec 6, 2021

Conversation

steff456
Copy link
Member

@steff456 steff456 commented Nov 10, 2021

This PR rewrites the sorting functions markdown file in a rst file.

Ref: #283

@steff456 steff456 requested review from rgommers and kgryte November 10, 2021 22:00
@steff456 steff456 self-assigned this Nov 10, 2021
@steff456 steff456 added the Maintenance Bug fix, typo fix, or general maintenance. label Nov 10, 2021
@rgommers
Copy link
Member

There's still an issue with the rendered docs:

image

signatures._sorting_functions. needs to go here.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Looks like a nice start, thanks @steff456!

Just a note here that we had a discussion about type annotations and whether they should be reflected in the function signatures or not (this is controlled by autodoc_typehints, see https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html). We thought it's good to start with the doing both - our type annotations are very clean, so it hopefully will look good in signatures.

@asmeurer
Copy link
Member

signatures._sorting_functions. needs to go here.

Do you mean the extra space after the ., or something else? The space is probably related to #139, which we will either have to fix by switching to another Sphinx theme or figuring out how to fix it upstream.

@kgryte
Copy link
Contributor

kgryte commented Nov 15, 2021

@asmeurer I think he meant the entirety of signatures._sorting_functions. so that the spec just shows sort(...).

@steff456
Copy link
Member Author

This PR is ready for review again!

Now the page of each function is shown like this,
image

@steff456
Copy link
Member Author

steff456 commented Dec 1, 2021

With the latest changes, now the type hint for the return type is shown as the parameters and the Return type section is removed.

image

I think now this is ready!

@kgryte
Copy link
Contributor

kgryte commented Dec 6, 2021

Based on the screenshot and looking through the changes, looks good to me.

@kgryte kgryte added this to the v2022 milestone Dec 6, 2021
@kgryte
Copy link
Contributor

kgryte commented Dec 6, 2021

I think we can go ahead and merge this, and, if there are any further changes, we can address in follow-up PRs. Thanks, @steff456!

@kgryte kgryte merged commit 657a8f4 into data-apis:main Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Bug fix, typo fix, or general maintenance.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants