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

Improve reference docs #7279

Merged
merged 10 commits into from
Oct 4, 2023
Merged

Conversation

NickCrews
Copy link
Contributor

Just some small things that have been missing from the docs that have been bothering me for a while.

@NickCrews NickCrews force-pushed the important-docstrings branch from ee56306 to 141d966 Compare October 2, 2023 19:01
ibis/expr/api.py Outdated Show resolved Hide resolved
This is to make these other APIs more discoverable.
make these more easily findable/understandable
by pandas users
Before it could be interpreted that this
returned Column objects. But it just returns
string names of the columns.

Per review comment, I'm not putting in the full
`list[str]` type annotation because it should get
rendered in quartodoc, it just isn't.
This is such a central class that it should have a docstring.
@NickCrews NickCrews force-pushed the important-docstrings branch from 812bf59 to f0a5a2e Compare October 3, 2023 17:08
@NickCrews
Copy link
Contributor Author

Thanks for the review @cpcloud! I applied all your suggestions.

@@ -116,21 +184,26 @@ def cast(self, target_type: dt.DataType) -> Value:

return op.to_expr()

def try_cast(self, target_type: dt.DataType) -> Value:
def try_cast(self, target_type: Any) -> Value:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IDK if you saw this type hint change @cpcloud. The old one was overly-restrictive. It might be nice if the dtype module exported some type alias for TypeLike = Union[dt.DataType, str, np.dtype, ...] so that here and in other places we could consume it.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine for now. We may want to use protocols for this specific case, as SupportsDataType at the user level is a well-defined set of things (internals are different story, there are a ton of things that are convertible to ibis DataTypes).

the one set of examples wasn't rendering because
interactive mode was off, and it wasn't
very useful anyway because it wasn't
acting on concrete data.
@NickCrews NickCrews force-pushed the important-docstrings branch from f0a5a2e to 199dafd Compare October 3, 2023 17:22
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.

Nice!

@cpcloud cpcloud added this to the 7.1 milestone Oct 4, 2023
@cpcloud cpcloud added the docs Documentation related issues or PRs label Oct 4, 2023
@cpcloud cpcloud merged commit d4c97b0 into ibis-project:master Oct 4, 2023
@NickCrews
Copy link
Contributor Author

@cpcloud
Per machow/quartodoc#284, we could get type annotations on Table.columns by adding

Attributes
----------
columns
        a copy of the existing docstring

to Table.__doc__. What do you think, is this duplication worth it?

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.

2 participants