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

Document some undocumented items #1707

Merged
merged 8 commits into from
Jul 4, 2021
Merged

Document some undocumented items #1707

merged 8 commits into from
Jul 4, 2021

Conversation

nw0
Copy link
Contributor

@nw0 nw0 commented Jun 30, 2021

This PR mainly focuses on the currently undocumented items (#306).

I've also added an #[allow(missing_docs)] to the ffi module.

The outstanding items seem to mainly be the protocols and exceptions macros, but I'm not very sure as my rustdoc reports 459 remaining items.

I think the exceptions macros need some love: will update the issue if and when I think of something reasonable.

Copy link
Member

@messense messense left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thank you very much for it, it's much appreciated!

I only have one nit, which is that I've changed the tense used e.g. "Return a pointer" to "Returns a pointer".

This is the same tense used by Rust's stdlib and I was thinking it's nice to use the same style here. Admittedly this is undocumented, we could perhaps add a docs style note somewhere in Contributing.md....

src/buffer.rs Outdated Show resolved Hide resolved
src/buffer.rs Outdated Show resolved Hide resolved
src/exceptions.rs Outdated Show resolved Hide resolved
src/exceptions.rs Outdated Show resolved Hide resolved
src/exceptions.rs Outdated Show resolved Hide resolved
src/types/datetime.rs Outdated Show resolved Hide resolved
src/types/datetime.rs Outdated Show resolved Hide resolved
src/types/datetime.rs Outdated Show resolved Hide resolved
src/types/sequence.rs Outdated Show resolved Hide resolved
src/types/string.rs Outdated Show resolved Hide resolved
src/buffer.rs Outdated Show resolved Hide resolved
src/ffi/mod.rs Outdated Show resolved Hide resolved
src/pyclass_slots.rs Outdated Show resolved Hide resolved
src/pyclass_slots.rs Outdated Show resolved Hide resolved
src/pyclass_slots.rs Outdated Show resolved Hide resolved
src/pyclass_slots.rs Outdated Show resolved Hide resolved
Co-authored-by: Georg Brandl <[email protected]>
Co-authored-by: David Hewitt <[email protected]>
@nw0
Copy link
Contributor Author

nw0 commented Jul 4, 2021

Accepted all suggestions.

Admittedly this is undocumented, we could perhaps add a docs style note somewhere in Contributing.md....

Contributing.md is pretty high-level about documentation -- it's quite clear about the the important points imo. I haven't added a note, but would be happy to write something, or if you want to push an additional thing to this PR.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Perfect, thanks again!

Contributing.md is pretty high-level about documentation -- it's quite clear about the the important points imo. I haven't added a note, but would be happy to write something, or if you want to push an additional thing to this PR.

Agreed, though also I guess that not many contributors will be writing massive docs PRs once we have everything present. And as we increase docs coverage it's likely that new contributions will match the existing style. Perhaps let's pass on the note for now.

@davidhewitt davidhewitt merged commit fde4211 into PyO3:main Jul 4, 2021
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.

4 participants