Skip to content

Add top level documentation comment template to cargo new #4855

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

Closed
wants to merge 2 commits into from
Closed

Add top level documentation comment template to cargo new #4855

wants to merge 2 commits into from

Conversation

vignesh-sankaran
Copy link
Contributor

@vignesh-sankaran vignesh-sankaran commented Dec 22, 2017

Work on #3506.

So the thing I'm not sure about is what the documentation comment template should look like with binary projects, since the way documentation is used for those projects may be different to a library. An example that comes to mind is perhaps prodding the user to list some sample commands the binary crate could run.

@sanmai-NL @carols10cents

@rust-highfive
Copy link

r? @matklad

(rust_highfive has picked a reviewer for you, use r? to override)

@matklad
Copy link
Member

matklad commented Dec 22, 2017

cc @rust-lang/cargo

I think we might discuss a little bit how this feature should be surfaced. I am afraid that for typical "let's create a hello world in Rust" use case this actually will be a misfeature. As an anecdote, I don't find the current lib template with a test module really useful either: I always have to delete it after the new project is created.

@sanmai-NL
Copy link

sanmai-NL commented Dec 22, 2017

On the one hand, adding the template doc comment could stimulate properly documenting new code, just like adding the test code template could for testing. On the other hand, I recognize @matklad’s experience in that I usually remove those things at the beginning for more immediately important lines (e.g., actual code.). Adding tests and docs happens very soon in further development of course. The programmer should perhaps be prodded to do so as lint checks (this happens for docs already).

@vignesh-sankaran: I think your intention is to lower the barrier to learning all kinds of details that are relevant to practical Rust programming. Have you looked at https://learnxinyminutes.com/docs/rust/? Is pointing out such a template page maybe a better approach than enriching cargo new’s output?

In relation to @matklad’s comment, I think the current cargo new lib template can be rationalized. What do we really want as template? We could offer two templates:

  • ‘minimal’ (even less lines than currently; default) and
  • ‘rich’ (alternative, which we then try to keep in sync with the external ‘Learn Rust in Y Minutes’ one or some other popular template).

This way ‘cargo new’ is helpful to more seasoned Rust developers as well as novices who enjoy having the template around when starting a new project. The latter could be a big deal when Rust is taught as part of courses, and to help learners cross the barrier to entry in their first projects.

Copy link
Member

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

A few bits of feedback to make it conform with our doc style.

I'm also a bit skeptical of the main.rs, honestly.

@@ -503,12 +503,36 @@ authors = [{}]

let default_file_content : &[u8] = if i.bin {
b"\
//! This is the top-level module documentation in src/main.rs.
//! This text appears on the front page of your crate's documentation;
Copy link
Member

Choose a reason for hiding this comment

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

There should be an empty line between this and the line above it.

//! This text appears on the front page of your crate's documentation;
//! preview it by running `cargo doc --open`.
//!
//! # Example
Copy link
Member

Choose a reason for hiding this comment

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

This should be Examples.

//! ```
//! assert(true);
//! ```

Copy link
Member

Choose a reason for hiding this comment

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

this newline should not be here

fn main() {
println!(\"Hello, world!\");
}
"
} else {
b"\
//! This is the top-level module documentation in src/lib.rs.
//! This text appears on the front page of your crate's documentation;
Copy link
Member

Choose a reason for hiding this comment

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

There should be a newline above this line

//! This text appears on the front page of your crate's documentation;
//! preview it by running `cargo doc --open`.
//!
//! # Example
Copy link
Member

Choose a reason for hiding this comment

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

Examples

//! ```
//! assert(true);
//! ```

Copy link
Member

Choose a reason for hiding this comment

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

✂️ the newline

@vignesh-sankaran
Copy link
Contributor Author

@sanmai-NL I'd say my intent is more along the lines of wanting to encourage new crates to have doc comments. I feel there is a place for a beginner-friendly template, is it worth adding a CLI flag to cargo itself for this though? I'm worried about the maintenance overhead that this will add and how to gauge if this is something that'd be of value to beginners.

My main worry right now is if we go ahead with this approach, should we be detecting if crate authors are publishing their crates without changing the default message? e.g. have a warning flash up stating if the default message is left unchanged, or as an extreme measure, prevent the crate from being published until the message is changed.

We could also perhaps offer an flag for no doc comments, or make that the default, and offer a doc comment template with a flag? But then I feel that defeats the purpose of having the template option in the first place since it won't be nearly as commonly used compared to having doc comments as the default option.

I feel we shouldn't really be having default doc comments for main.rs, is this the thing you're skeptical about @steveklabnik?

Btw, I am aware of the failing tests, I'll go modify them once we've decided on an approach for this PR :).

@joshtriplett
Copy link
Member

You should absolutely prevent people from uploading crates with the default doc comment.

@sanmai-NL
Copy link

@joshtriplett: Seems unworkable. What if authors only change the doc comment superficially? @vignesh-sankaran: Let’s just not add the the default doc comment at all then. Do I understand correctly you prefer to keep it for lib.rs? I don’t see why we should.

@joshtriplett
Copy link
Member

joshtriplett commented Dec 23, 2017

What if authors only change the doc comment superficially?

Then they won't get a warning. I'm suggesting that if it stays completely unmodified they should get a warn-by-default lint. (I almost said "deny-by-default", but sometimes it's useful to cargo new --bin foo to create a quick-and-dirty hack, and let's not add extra steps like "delete the comment" or "pass --no-something" to that.)

The goal is to catch common errors.

@matklad
Copy link
Member

matklad commented Dec 23, 2017

I'd say my intent is more along the lines of wanting to encourage new crates to have doc comments.

Not every new crate needs a doc comment though: a lot of crates are just private stuff which you never publish to crates.io.

Perhaps the better way to achieve a desired result is to add some checks before publication?

For example, cargo publish could check if the crate is to be uploaded for the first time, and in this case warn once about missing docs, missing fields in Cargo.toml, etc.

@vignesh-sankaran
Copy link
Contributor Author

Sounds like a good approach. I'm currently working on this, it may take me a few days to put things together :).

@vignesh-sankaran
Copy link
Contributor Author

Ok so I'm closing this PR for the reasons outlined in #3506.

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.

6 participants