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

docs(reference): improve signatures #7262

Merged
merged 5 commits into from
Oct 1, 2023

Conversation

NickCrews
Copy link
Contributor

@NickCrews NickCrews commented Sep 29, 2023

Followup to #7159

Make the signatures appear show the full path,
eg random() is now ibis.random().

Also fix some of the paths where possible,
eg now it is ibis.connect() instead of backends.base.connect()

@machow, note how quartodoc was silently ignoring the previously-incorrectly-configured signature_name option. It would have been nice if the build had failed here, instead of us thinking that we
had configured something.

See the difference in ibis.connect():
image

and note how in the sidebar you just see "random", not "ibis.random"
image

@NickCrews NickCrews mentioned this pull request Sep 29, 2023
1 task
@cpcloud
Copy link
Member

cpcloud commented Sep 30, 2023

Not really a huge fan of showing the full module path for methods:

image

For top-level functions I think we should show the ibis module prefix, but for everything else the module where the object resides is 1) mostly irrelevant for end-users, and 2) subject to change at any time and not part of the public API in any way.

@cpcloud
Copy link
Member

cpcloud commented Sep 30, 2023

I'm looking into ways to render the longer name selectively

NickCrews and others added 5 commits September 30, 2023 08:56
This has no public-facing changes. It just simplifies how we set the
package for each item in reference.
Now everything has the default of being underneath the
top-level "ibis" unless
overridden within a page,
section, or member.
When looking at the reference, some methods are listed by where they
are implemented, not where they are exposed publicly. eg
`ibis.connect()` signature is listed as
`backends.base.connect(resource, **kwargs)`
currently. This commit changes that to simple
`connect(resource, **kwargs)`
where possible.

For some reason I couldn't do this
for `ibis.NA`, not sure why...
GroupedTable is actually defined in the `ibis.expr.types.groupby`
module, not the `ibis.expr.types.relations` module.
Before, quartodoc was noticably slower to build this page of
documentation than the other pages,
because it looked in the wrong
place and then had to do another
import to actually get to the
right place. Before it took
~10s to build this page, now it
is instantaneous.
Not a big deal, but is nice to
iterate faster when working on
the reference docs.
Now the signatures appear as
`ibis.random()` instead of just `random()`.
Per
ibis-project#7159 (comment)

Before, quartodoc wasn't even respecting
this option, because it was incorrectly
definied at the wrong nesting level.
I had to move the option out of the "member_options" section
for any change to occur.
@cpcloud cpcloud force-pushed the docs-ibis-signature branch from 4a6e345 to e994026 Compare September 30, 2023 12:56
@cpcloud cpcloud added this to the 7.0 milestone Sep 30, 2023
@cpcloud cpcloud added the docs Documentation related issues or PRs label Sep 30, 2023
@cpcloud
Copy link
Member

cpcloud commented Sep 30, 2023

Alright, I was able to specify signature_name: full for all the top level functions.

Unfortunately I can't specify signature_name: full at the top of the config and then specify signature_name: short for the objects I want to have short signatures, because that option doesn't cascade to auto-discovered methods.

@NickCrews
Copy link
Contributor Author

This is better, thank you!

@cpcloud cpcloud merged commit 9793862 into ibis-project:master Oct 1, 2023
@machow
Copy link
Contributor

machow commented Oct 2, 2023

Sorry, just getting to this. If I'm understanding the original docs did this:

  • Set default member_options, so that methods on the automatically discovered classes (e.g. - TimestampValue in contents) showed short signatures.
  • Everything else used the default behavior of showing whatever is specified as name: .

I wonder if this strategy would work?

  • set package: null (instead of ibis) in the top-level quartodoc config
  • set member_options with signature_name: short again
  • remove the default of display_name: short
  • replace entries that used package: ibis to just include ibis in the name:

So something like...

# 
quartodoc:
  package: ibis
  sections:
    ...
            - name: read_delta
              dynamic: true

becomes

quartodoc:
  package: null
  sections:
    ...
            - name: ibis.read_delta
              dynamic: true

It seems like getting the config right here is tricky, because one set of things (e.g. GroupedTable) are displayed in a way that is less centered on how you might import them (i.e. if you see a GroupedTable, here are the methods you might be able to use), while the others need to make clear where they can be imported from (e.g. you can't import ibis.GroupedTable, but you probably want people to realize right away they can import ibis.read_json)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related issues or PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants