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: add callout notes for some APIs changes and additions #10551

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

NickCrews
Copy link
Contributor

@NickCrews NickCrews commented Dec 3, 2024

fixes #10535

Other things I considered:

using some other mechanism, like sphinx versionadded directives

like https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-versionadded

but I don't think our doc system uses sphinx. So I think we are limited to using quarto's constructs?

using a different callout type

There is warning, tip, etc. I thought note made the most sense.

Adding a @versionadded decorator

Similar to the @experimental and @deprecated decorators in util.py. But I thought adding them inline like this wasn't that hard.

Placing the callout in a different place in the docstring

eg further up, where it might be more obvious. But the official python docs put them down at the bottom, so I chose to follow that convention. I think that future-proofs us if there are multiple callouts in a row.

Some sort of autocheck

In future PRs, how can we be sure that docstrings are updated like this? I couldn't think of a good way to do it. Maybe if there is a breaking-change tag on the issue? but that doesn't work for Added in version changes. I think the best we can do is to just have all maintainers be aware of this. If we like this approach then we can tag in all the maintainers into this issue?

@github-actions github-actions bot added the duckdb The DuckDB backend label Dec 3, 2024
@NickCrews NickCrews added the docs-preview Add this label to trigger a docs preview label Dec 6, 2024
@ibis-docs-bot ibis-docs-bot bot removed the docs-preview Add this label to trigger a docs preview label Dec 6, 2024
@ibis-docs-bot
Copy link

ibis-docs-bot bot commented Dec 6, 2024

@NickCrews
Copy link
Contributor Author

Hmm, I think actually it's a bit hard to see down at the bottom. Maybe it would be better if the notes were right before the Parameters section so it's easier to see, it doesn't take up much room. If there were every any detailed notes, the call-out would start collapsed?

Screenshot_20241207-113257.png

@NickCrews
Copy link
Contributor Author

@cpcloud @gforsyth this is ready for review whenever you have a chance! I think I am in favor of moving the call-out to just before the Parameters section, but wanted your confirmation before I made that effort.

@NickCrews
Copy link
Contributor Author

@IndexSeek you had chimed in on the related issue, do you have any thoughts here?

@IndexSeek
Copy link
Member

I agree. It might make more sense to move it above the parameters section. That's a good place for it.

We don't have any Quarto callout blocks in the other docstrings. I have seen the Sphinx directives used in docstrings before; I was curious to know if this was common.

I think this is a valuable change, but I want to ensure we're not undoing #5347 as you commented on the other issue.

@gforsyth
Copy link
Member

I'll try to take a closer look this week. Broadly on board with adding this information and I think above parameters is a fine place for it.

I don't think it's counter to #5347 -- having version related information is fine, we just didn't want the headache of publishing versioned docs.

@NickCrews NickCrews force-pushed the version-added-callouts branch from e650032 to 7ec3eda Compare December 13, 2024 06:28
Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

Other than the spurious additional newline, LGTM!

ibis/backends/duckdb/__init__.py Outdated Show resolved Hide resolved
@cpcloud cpcloud added this to the 10.0 milestone Dec 20, 2024
@cpcloud cpcloud added the docs Documentation related issues or PRs label Dec 21, 2024
@cpcloud cpcloud force-pushed the version-added-callouts branch from e965a1a to 640163e Compare December 21, 2024 10:43
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 duckdb The DuckDB backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: cases() documentation doesn't work for me at all
4 participants