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

Require docs for public members #427

Merged
merged 1 commit into from
Nov 3, 2023
Merged

Conversation

jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Oct 29, 2023

Added #[allow(missing_docs)] for quite a few of the impl enums where I would struggle to write useful docs.

Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

Much appreciated; please see comments.

typify-impl/src/lib.rs Outdated Show resolved Hide resolved
typify-impl/src/lib.rs Outdated Show resolved Hide resolved
@@ -902,13 +919,15 @@ impl<'a> TypeEnum<'a> {
}

impl<'a> TypeStruct<'a> {
/// Get name and type of each property.
pub fn properties(&'a self) -> impl Iterator<Item = (&'a str, TypeId)> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm probably going to EOL this interface in the future.

typify-impl/src/lib.rs Outdated Show resolved Hide resolved
typify-impl/src/lib.rs Outdated Show resolved Hide resolved
typify-impl/src/lib.rs Show resolved Hide resolved
@@ -94,13 +101,15 @@ pub struct TypeEnum<'a> {
details: &'a type_entry::TypeEntryEnum,
}

#[allow(missing_docs)]
/// Enum variant details.
pub enum TypeEnumVariant<'a> {
Simple,
Tuple(Vec<TypeId>),
Struct(Vec<(&'a str, TypeId)>),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Struct(Vec<(&'a str, TypeId)>),
/// Struct-type variant with named properties and types.
Struct(Vec<(&'a str, TypeId)>),

Ugh... makes me realize that there's no way to get at the description for each variant.

@@ -94,13 +101,15 @@ pub struct TypeEnum<'a> {
details: &'a type_entry::TypeEntryEnum,
}

#[allow(missing_docs)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#[allow(missing_docs)]

@@ -70,6 +76,7 @@ pub struct Type<'a> {
type_entry: &'a TypeEntry,
}

#[allow(missing_docs)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can document this; see inspiration below. Let me know if you'd like me to add suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the ones you've given, and added a few more.
My intention was to put #[allow(missing_docs)] where other people can replace it with real docs.

The goal of the PR isnt to document everything, but instead to ensure new enhancements must include docs at the outset.

@@ -38,6 +42,7 @@ pub struct CliArgs {
}

impl CliArgs {
/// Output path.
pub fn output_path(&self) -> Option<PathBuf> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably just nothing in this file should be pub since it's a command...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CliArgs's struct fields should be documented as those docs appear on the command line --help.
IMO it is simpler to write some basic docs, rather than having guidelines for contributors when #[allow(missing_docs)] is allowed and when it isn't.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point!

@ahl ahl merged commit 5f9235d into oxidecomputer:main Nov 3, 2023
4 checks passed
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.

2 participants