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

Ensure all APIs are documented (#![deny(missing_docs)]) #306

Closed
konstin opened this issue Dec 8, 2018 · 6 comments · Fixed by #2588
Closed

Ensure all APIs are documented (#![deny(missing_docs)]) #306

konstin opened this issue Dec 8, 2018 · 6 comments · Fixed by #2588

Comments

@konstin
Copy link
Member

konstin commented Dec 8, 2018

We have two main parts of the documentation, the guide and the api docs, which both could be much better. The vision is to have a complete guide for pyo3, similar to the cargo book, and documentation on a #![deny(missing_docs)] level with many examples.

The guide currently only covers a small part of all features, e.g. conversions, type objects or the lifetime concept aren't explained at all. So if you've used some feature not documented in the guide, it would be great if you'd send pull request with an explanation and/or an example.

The api docs are also sparse, with mostly very short comments, almost no examples and no explanations of the relationships between the different types and traits. So if you describe an api better for new user, know the interactions between the types or have a good example, a pull request is highly appreciated!

@davidhewitt
Copy link
Member

I'm about to push an update to CONTRIBUTING.md which will include the above note, so I'm going to narrow this issue to focus on #![deny(missing_docs)].

@davidhewitt davidhewitt changed the title Improve the documentation Ensure all APIs are documented (#![deny(missing_docs)]) Aug 6, 2020
@nw0
Copy link
Contributor

nw0 commented Jun 29, 2021

Should we enforce documentation for (suggestions included):

  • FFI functions? I think not at all -- one link at the top of the module should suffice
  • Protocols? I'd insert a link in the trait doc, but leave the method docs empty
  • Implementations which shadow Python APIs, such as PyBuffer? I'm inclined to say we should fully document these, even if most of the methods essentially wrap Python APIs.
  • The macro-generated pyo3::exceptions::*. Not really sure what to make of this. I don't particularly want to slap the same docstring onto all the new_err methods, because then it feels like it should be a trait.

I've written minimal documentation for most of the remaining functions (hard to tell, because of all the noise). If I could get an opinion on the 3 areas above, I'll try to file a PR soon.

@davidhewitt
Copy link
Member

davidhewitt commented Jun 29, 2021

FFI functions? I think not at all -- one link at the top of the module should suffice

Agreed. We don't expose the FFI sub-modules at all, so just a top level pyo3::ffi doc is likely to be enough.

Protocols? I'd insert a link in the trait doc, but leave the method docs empty

Sounds fine, note that we're probably going to kill protocols for the 0.15 release (see #1206), so don't put too much effort into these.

Implementations which shadow Python APIs, such as PyBuffer? I'm inclined to say we should fully document these, even if most of the methods essentially wrap Python APIs.

Agreed. Even if they might be "obvious" to us experienced users, I think a library really feels like it's significantly higher quality to new users if literally every method has docs.

The macro-generated pyo3::exceptions::*. Not really sure what to make of this. I don't particularly want to slap the same docstring onto all the new_err methods, because then it feels like it should be a trait.

Maybe it should be a trait 😄. That's a question for another time though.

TBH for now you could always add #[allow(missing_docs)] in some of these cases. I think with justification we can allow this for certain parts of the codebase (e.g. all of pyo3::ffi).

Regarding enforcing #[deny(missing_docs)]: I was originally thinking we could add it as an attribute at the top of lib.rs, but having used this in private codebases I tend to find it gets in the way of prototyping a bit. The alternative could be to only enforce it in CI, similar to how we do #[deny(warnings)].

@nw0
Copy link
Contributor

nw0 commented Jun 30, 2021

Sounds fine, note that we're probably going to kill protocols for the 0.15 release (see #1206), so don't put too much effort into these.

Good to know. If you don't mind I'm just going to open a PR first and look at the protocols again once it's clear which ones are going away.

@nw0
Copy link
Contributor

nw0 commented Jul 5, 2021

Are the pyobject_native_type_X (and pyobject_native_type!) macros meant to be part of this library's public API? Seems like we should have defined all the relevant types in the ffi section.

@davidhewitt
Copy link
Member

Are the pyobject_native_type_X (and pyobject_native_type!) macros meant to be part of this library's public API?

In general I have been not included changes to them in the CHANGELOG, and consider them internal APIs. I think such macros could be useful however I suspect they're very specialized for certain use cases - really we should be providing APIs for core types in pyo3::types. Also I kind of expect to need to break them at least for #1308 and maybe #1344, so I don't really want to encourage downstream to use them yet.

Seems like we should have defined all the relevant types in the ffi section.

Which types do you mean?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants