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

Enhance tiledb.cloud.dag module typing and documentation #601

Merged
merged 4 commits into from
Jul 15, 2024

Conversation

spencerseale
Copy link
Collaborator

@JohnMoutafis
Copy link
Contributor

JohnMoutafis commented Jul 12, 2024

@spencerseale Have you tested with quartodoc locally how the docstrings will appear in the docs?

@spencerseale
Copy link
Collaborator Author

@spencerseale Have you tested with quartodoc locally how the docstrings will appear in the docs?

Thanks @JohnMoutafis, yes it looks as I'd like it locally and we now have all the important classes included in the api ref for the low-level task graph api

@spencerseale
Copy link
Collaborator Author

DAG.submit is not getting included in the doc build because it is defined via submit = submit_udf.

For users, this may be confusing as many of our tutorials reference submit, not submit_udf.

Can we remove this redef and just rename the original submit_udf to submit? @thetorpedodog @JohnMoutafis @ihnorton

Copy link
Collaborator

@sgillies sgillies left a comment

Choose a reason for hiding this comment

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

@spencerseale thanks for taking this on! Good docstrings are great for our users.

I've made a suggestion for general comment style. I think that many of these comments are clear from the code and can be omitted.

Many type hints have been added. It would be good to update the title of the PR to reflect that so that reviewers understand the intent as they begin.

self.id = uuid.uuid4()
"""UUID for Node instance."""
Copy link
Collaborator

@sgillies sgillies Jul 12, 2024

Choose a reason for hiding this comment

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

@spencerseale what would you think about using Python comments instead of unassigned strings for this PR?

Suggested change
"""UUID for Node instance."""
# UUID for Node instance.

I actually think many of the comments you've added are, while accurate, clear from the code and can be omitted. Less is more. They won't surface in documentation.

Copy link
Collaborator Author

@spencerseale spencerseale Jul 12, 2024

Choose a reason for hiding this comment

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

These """ """ are just meant to materialize the attribute definitions in the docs rather than for someone looking through the code. I definitely agree on not overdoing the comments! My main intent was to get users all information about these classes into the api ref, where currently there is nothing: https://tiledb-inc.github.io/TileDB-Cloud-Py/reference/dag.dag.html

This is what the attributes look like in the api ref now with these: Screenshot 2024-07-12 at 3 27 41 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

@spencerseale Great! I had forgotten that attribute docstrings are a thing. In https://discuss.python.org/t/revisiting-attribute-docstrings/36413 I see that they're pretty widely used and appreciated. I'll amend my review.

@spencerseale spencerseale changed the title add doc strings for dag module Enhance tiledb.cloud.dag module typing and documentation Jul 12, 2024
Copy link
Collaborator

@sgillies sgillies left a comment

Choose a reason for hiding this comment

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

My initial review was made from a place of ignorance regarding attribute docstrings. I'm in favor of them.

@spencerseale spencerseale merged commit 6f8afa6 into main Jul 15, 2024
18 checks passed
@spencerseale spencerseale deleted the spencerseale/sc-50841/dag-doc-str branch July 15, 2024 16:20
@spencerseale spencerseale self-assigned this Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants