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

Default values for readme if not specified #8277

Merged
merged 15 commits into from
Jun 9, 2020
18 changes: 18 additions & 0 deletions crates/cargo-test-support/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1622,6 +1622,24 @@ pub fn basic_lib_manifest(name: &str) -> String {
)
}

pub fn basic_bin_manifest_with_readme(name: &str, readme_filename: &str) -> String {
alexcrichton marked this conversation as resolved.
Show resolved Hide resolved
format!(
r#"
[package]

name = "{}"
version = "0.5.0"
authors = ["[email protected]"]
readme = {}

[[bin]]

name = "{}"
"#,
name, readme_filename, name
)
}

pub fn path2url<P: AsRef<Path>>(p: P) -> Url {
Url::from_file_path(p).ok().unwrap()
}
Expand Down
31 changes: 29 additions & 2 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,7 @@ pub struct TomlProject {
description: Option<String>,
homepage: Option<String>,
documentation: Option<String>,
readme: Option<String>,
readme: Option<StringOrBool>,
keywords: Option<Vec<String>>,
categories: Option<Vec<String>>,
license: Option<String>,
Expand Down Expand Up @@ -1198,11 +1198,12 @@ impl TomlManifest {
project.links.as_deref(),
project.namespaced_features.unwrap_or(false),
)?;

let metadata = ManifestMetadata {
description: project.description.clone(),
homepage: project.homepage.clone(),
documentation: project.documentation.clone(),
readme: project.readme.clone(),
readme: readme_for_project(package_root, project),
authors: project.authors.clone().unwrap_or_default(),
license: project.license.clone(),
license_file: project.license_file.clone(),
Expand Down Expand Up @@ -1513,6 +1514,32 @@ impl TomlManifest {
}
}

/// Returns the name of the README file for a `TomlProject`.
fn readme_for_project(package_root: &Path, project: &TomlProject) -> Option<String> {
match &project.readme {
None => default_readme_from_package_root(package_root),
Some(value) => match value {
StringOrBool::Bool(false) => None,
StringOrBool::Bool(true) => default_readme_from_package_root(package_root),
Copy link
Member

Choose a reason for hiding this comment

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

This is an interesting case because readme = true could actually still infer no actual README if the files are missing. Perhaps for now we could just make readme = true an error? Otherwise we probably want to assert that the default readme is indeed present, and None isn't returned in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay in getting back to this.

What do you think of this approach: Similar to how the build field is handled, if readme = true, we assume a default value of README.md (instead of possibly None, as you pointed out). Then, we can validate that the file really exists while creating the ManifestMetadata, and bail! if the file doesn't exist. This would be more consistent with the behavior of the build field, but I'm not sure how important that is.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that sounds reasonable to me!

StringOrBool::String(v) => Some(v.clone()),
},
}
}

const DEFAULT_README_FILES: [&str; 3] = ["README.md", "README.txt", "README"];

/// Checks if a file with any of the default README file names exists in the package root.
/// If so, returns a `String` representing that name.
fn default_readme_from_package_root(package_root: &Path) -> Option<String> {
for &readme_filename in DEFAULT_README_FILES.iter() {
if package_root.join(readme_filename).is_file() {
return Some(readme_filename.to_string());
}
}

None
}

/// Checks a list of build targets, and ensures the target names are unique within a vector.
/// If not, the name of the offending build target is returned.
fn unique_build_targets(targets: &[Target], package_root: &Path) -> Result<(), String> {
Expand Down
5 changes: 5 additions & 0 deletions src/doc/src/reference/manifest.md
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,11 @@ will interpret it as Markdown and render it on the crate's page.
readme = "README.md"
```

If no value is specified for this field, and a file named `README.md`,
`README.txt` or `README` exists in the package root, then the name of that
file will be used. You can suppress this behavior by setting this field to
`false`.

#### The `homepage` field

The `homepage` field should be a URL to a site that is the home page for your
Expand Down
96 changes: 85 additions & 11 deletions tests/testsuite/read_manifest.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
//! Tests for the `cargo read-manifest` command.

use cargo_test_support::{basic_bin_manifest, main_file, project};
use cargo_test_support::{basic_bin_manifest, basic_bin_manifest_with_readme, main_file, project};

static MANIFEST_OUTPUT: &str = r#"
{
fn manifest_output(readme_value: &str) -> String {
format!(
r#"
{{
"authors": [
"[email protected]"
],
"categories": [],
"name":"foo",
"readme": null,
"readme": {},
"repository": null,
"version":"0.5.0",
"id":"foo[..]0.5.0[..](path+file://[..]/foo)",
Expand All @@ -21,19 +23,26 @@ static MANIFEST_OUTPUT: &str = r#"
"edition": "2015",
"source":null,
"dependencies":[],
"targets":[{
"targets":[{{
"kind":["bin"],
"crate_types":["bin"],
"doctest": false,
"edition": "2015",
"name":"foo",
"src_path":"[..]/foo/src/foo.rs"
}],
"features":{},
}}],
"features":{{}},
"manifest_path":"[..]Cargo.toml",
"metadata": null,
"publish": null
}"#;
}}"#,
readme_value
)
}

fn manifest_output_no_readme() -> String {
manifest_output("null")
}

#[cargo_test]
fn cargo_read_manifest_path_to_cargo_toml_relative() {
Expand All @@ -44,7 +53,7 @@ fn cargo_read_manifest_path_to_cargo_toml_relative() {

p.cargo("read-manifest --manifest-path foo/Cargo.toml")
.cwd(p.root().parent().unwrap())
.with_json(MANIFEST_OUTPUT)
.with_json(&manifest_output_no_readme())
.run();
}

Expand All @@ -58,7 +67,7 @@ fn cargo_read_manifest_path_to_cargo_toml_absolute() {
p.cargo("read-manifest --manifest-path")
.arg(p.root().join("Cargo.toml"))
.cwd(p.root().parent().unwrap())
.with_json(MANIFEST_OUTPUT)
.with_json(&manifest_output_no_readme())
.run();
}

Expand Down Expand Up @@ -104,5 +113,70 @@ fn cargo_read_manifest_cwd() {
.file("src/foo.rs", &main_file(r#""i am foo""#, &[]))
.build();

p.cargo("read-manifest").with_json(MANIFEST_OUTPUT).run();
p.cargo("read-manifest")
.with_json(&manifest_output_no_readme())
.run();
}

#[cargo_test]
fn cargo_read_manifest_with_specified_readme() {
let p = project()
.file(
"Cargo.toml",
&basic_bin_manifest_with_readme("foo", r#""SomeReadme.txt""#),
)
.file("SomeReadme.txt", "Sample Project")
.file("src/foo.rs", &main_file(r#""i am foo""#, &[]))
.build();

p.cargo("read-manifest")
.with_json(&manifest_output(&format!(r#""{}""#, "SomeReadme.txt")))
.run();
}

#[cargo_test]
fn cargo_read_manifest_default_readme() {
let readme_filenames = ["README.md", "README.txt", "README"];

for readme in readme_filenames.iter() {
let p = project()
.file("Cargo.toml", &basic_bin_manifest("foo"))
.file(readme, "Sample project")
.file("src/foo.rs", &main_file(r#""i am foo""#, &[]))
.build();

p.cargo("read-manifest")
.with_json(&manifest_output(&format!(r#""{}""#, readme)))
.run();
}
}

#[cargo_test]
fn cargo_read_manifest_suppress_default_readme() {
let p = project()
.file(
"Cargo.toml",
&basic_bin_manifest_with_readme("foo", "false"),
)
.file("README.txt", "Sample project")
.file("src/foo.rs", &main_file(r#""i am foo""#, &[]))
.build();

p.cargo("read-manifest")
.with_json(&manifest_output_no_readme())
.run();
}

// If a file named README.txt exists, and `readme = true`, the value `README.txt` should be defaulted in.
#[cargo_test]
fn cargo_read_manifest_defaults_readme_if_true() {
let p = project()
.file("Cargo.toml", &basic_bin_manifest_with_readme("foo", "true"))
.file("README.txt", "Sample project")
.file("src/foo.rs", &main_file(r#""i am foo""#, &[]))
.build();

p.cargo("read-manifest")
.with_json(&manifest_output(&format!(r#""{}""#, "README.txt")))
.run();
}