Relative links in README #1131

kivikakk · 2017-10-16T09:21:23Z

This is a proposed fix for #974.

Currently implemented is GitHub/GitLab/Bitbucket-style (insert /blob/master/ between the repository and the link destination), with a caveat.

Currently implemented is GitHub/GitLab/Bitbucket-style (insert /blob/master/ between the repository and the link destination), with a caveat.

kureuil

Nice changes, thanks for implementing this 👍

Just missing a few documentation comments to account for the new usage & some questions about validation.

kureuil · 2017-10-16T09:57:22Z

src/render.rs

@@ -11,7 +11,7 @@ pub struct MarkdownRenderer<'a> {

 impl<'a> MarkdownRenderer<'a> {
    /// Creates a new renderer instance.
-    pub fn new() -> MarkdownRenderer<'a> {
+    pub fn new(repo: Option<&'a str>) -> MarkdownRenderer<'a> {


Should this parameter be renamed to something like base_url to better reflect its usage ?
Also, its usage should be documented.

kureuil · 2017-10-16T09:58:13Z

src/render.rs

 /// ```
-pub fn markdown_to_html(text: &str) -> CargoResult<String> {
-    let renderer = MarkdownRenderer::new();
+pub fn markdown_to_html(text: &str, repo: Option<&str>) -> CargoResult<String> {


Can you add a note in the documentation about the intended usage & the expected format of the new parameter?

Will also do!

kureuil · 2017-10-16T10:00:46Z

src/render.rs

-    let renderer = MarkdownRenderer::new();
+pub fn markdown_to_html(text: &str, repo: Option<&str>) -> CargoResult<String> {
+    let repo = repo.map(|r|
+        format!("{}{}blob/master/", r, if r.ends_with("/") { "" } else { "/" }));


We already depend on the url crate, should we use it to check that the given repo url's scheme is either http or https? I think it is possible for someone to submit a crate with a repository link using the git://... scheme.

Also, would it be worth it to check that the hosts are either github.com, gitlab.com or bitbucket.org ?

should we use it to check that the given repo url's scheme is either http or https?

I've just looked into it; I think we're safe — the API already checks to ensure a repository has either http or https, and by the documentation on crates.io this is expected:

These are intended to be webviews of the relevant data, not necessarily compatible with VCS tools and the like.

Here's the relevant code:

crates.io/src/krate/mod.rs

Lines 183 to 219 in ef1ac58

fn validate(&mut self, license_file: Option<&'a str>) -> CargoResult<()> {

fn validate_url(url: Option<&str>, field: &str) -> CargoResult<()> {

let url = match url {

Some(s) => s,

None => return Ok(()),

};

let url = Url::parse(url).map_err(|_| {

human(&format_args!("`{}` is not a valid url: `{}`", field, url))

})?;

match &url.scheme()[..] {

"http" | "https" => {}

s => {

return Err(human(&format_args!(

"`{}` has an invalid url \

scheme: `{}`",

field,

s

)))

}

}

if url.cannot_be_a_base() {

return Err(human(&format_args!(

"`{}` must have relative scheme \

data: {}",

field,

url

)));

}

Ok(())

}

validate_url(self.homepage, "homepage")?;

validate_url(self.documentation, "documentation")?;

validate_url(self.repository, "repository")?;

self.validate_license(license_file)?;

Ok(())

}

I think it's a good idea to check the host; in case it doesn't match any, I feel like we should just drop relative links.

kivikakk · 2017-10-16T10:39:15Z

Finally got a rustfmt nightly matching that of crates.io's Travis config going, so this last push should be all greens.

kureuil

I just have two small questions, other than that this PR seems perfect to me ! :)

kureuil · 2017-10-16T12:28:50Z

Cargo.toml

@@ -55,7 +55,7 @@ serde = "1.0.0"
 clippy = { version = "=0.0.162", optional = true }
 chrono = { version = "0.4.0", features = ["serde"] }
 comrak = { version = "0.1.9", default-features = false }
-ammonia = "0.7.0"
+ammonia = { git = "https://github.com/notriddle/ammonia" }


Shouldn't this dependency be specified as { git = "https://github.com/notriddle/ammonia", rev = "46054ff" } to ensure reproducible builds ?

Good idea! I was thinking Cargo.lock storing the rev would be sufficient, but this is better.

kureuil · 2017-10-16T12:31:37Z

src/render.rs

-            // assert_eq!(result, "<p><a href=\"https://github.com/rust-lang/test/blob/master/hi\" rel=\"nofollow noopener noreferrer\">hi</a>");
+        for url in &[
+            "https://github.com/rust-lang/test",
+            "https://github.com/rust-lang/test/",


Should we have gitlab.com & bitbucket.org tests in here to avoid regressions ?

Yes, let's.

kivikakk · 2017-10-16T12:49:45Z

Done! Thank you so much for your feedback. ❤️

…inks

carols10cents · 2017-10-23T17:49:15Z

@kivikakk I think the resolving is being a bit too aggressive now, it's resolving absolute URLs too-- I added a test case that's currently failing to illustrate:

---- render::tests::more_relative_links stdout ----
	thread 'render::tests::absolute_links_dont_get_resolved' panicked at 'assertion failed: `(left == right)`
  left: `"<p><a href=\"https://github.com/kbknapp/clap-rs/blob/master/https://crates.io/crates/clap\" rel=\"nofollow noopener noreferrer\"><img src=\"https://github.com/kbknapp/clap-rs/blob/master/https://img.shields.io/crates/v/clap.svg\" alt=\"Crates.io\"></a></p>\n"`,
 right: `"<p><a href=\"https://crates.io/crates/clap\" rel=\"nofollow noopener noreferrer\"><img src=\"https://img.shields.io/crates/v/clap.svg\" /></a></p>\n"`', src/render.rs:309:8

And I was also wondering how this interacts with the anchor support in your other PR-- anchors shouldn't be resolved, correct? Having a test case for that would be good too :)

Other than those two concerns, this is looking good!!

kivikakk · 2017-10-26T02:00:34Z

@carols10cents Thank you so much for the test case! What a dork I am. I'll add the extra test case as you suggested and fix. 👍

…inks

kivikakk · 2017-10-26T05:58:05Z

Have resolved conflicts; looks like the overzealous/absolute URL application is possibly an upstream bug, as the custom UrlRelativeEvalutate shouldn't be called on absolute URLs. Looking into it!

kivikakk · 2017-10-26T06:20:46Z

-> https://github.com/notriddle/ammonia/pull/77

kivikakk · 2017-10-26T06:52:43Z

Ammonia fix merged upstream and this branch now cleaned up. :) An anchor test was already in master (from the user-content- prefixing test) so we're all good there.

carols10cents · 2017-10-29T20:38:11Z

Thanks!!! Working great now!!!

bors: r+

1131: Relative links in README r=carols10cents This is a proposed fix for #974. Currently implemented is GitHub/GitLab/Bitbucket-style (insert `/blob/master/` between the repository and the link destination), with a caveat.

bors-voyager · 2017-10-29T20:56:31Z

Build succeeded

continuous-integration/travis-ci/push

Relative links in README

2e810af

Currently implemented is GitHub/GitLab/Bitbucket-style (insert /blob/master/ between the repository and the link destination), with a caveat.

kivikakk mentioned this pull request Oct 16, 2017

Rendered intra-project README links are broken #974

Closed

kureuil suggested changes Oct 16, 2017

View reviewed changes

kivikakk added 2 commits October 16, 2017 21:25

Latest ammonia to fix outstanding case; document

7da6e40

rustfmt

82756da

kureuil suggested changes Oct 16, 2017

View reviewed changes

kivikakk added 2 commits October 16, 2017 23:41

lock ammonia rev in Cargo.toml

69405bc

include gitlab/bitbucket in tests

d9d00e2

carols10cents added 2 commits October 23, 2017 13:43

Add a failing test-- too much resolving now :)

a908260

Merge remote-tracking branch 'upstream/master' into relative-readme-l…

dc95419

…inks

carols10cents added the S-waiting-on-author label Oct 25, 2017

Merge remote-tracking branch 'upstream/master' into relative-readme-l…

60066dc

…inks

kivikakk added 2 commits October 26, 2017 17:39

Latest ammonia; clippy fixes

8b0d4e3

clippy & fmt

59d144a

some more docs

3f64fd3

bors-voyager bot merged commit 3f64fd3 into rust-lang:master Oct 29, 2017

kivikakk deleted the relative-readme-links branch October 30, 2017 03:55

Turbo87 removed the S-waiting-on-author label Dec 3, 2017

	fn validate(&mut self, license_file: Option<&'a str>) -> CargoResult<()> {
	fn validate_url(url: Option<&str>, field: &str) -> CargoResult<()> {
	let url = match url {
	Some(s) => s,
	None => return Ok(()),
	};
	let url = Url::parse(url).map_err(\|_\| {
	human(&format_args!("`{}` is not a valid url: `{}`", field, url))
	})?;
	match &url.scheme()[..] {
	"http" \| "https" => {}
	s => {
	return Err(human(&format_args!(
	"`{}` has an invalid url \
	scheme: `{}`",
	field,
	s
	)))
	}
	}
	if url.cannot_be_a_base() {
	return Err(human(&format_args!(
	"`{}` must have relative scheme \
	data: {}",
	field,
	url
	)));
	}
	Ok(())
	}

	validate_url(self.homepage, "homepage")?;
	validate_url(self.documentation, "documentation")?;
	validate_url(self.repository, "repository")?;
	self.validate_license(license_file)?;
	Ok(())
	}

Relative links in README #1131

Relative links in README #1131

Uh oh!

Conversation

kivikakk commented Oct 16, 2017

Uh oh!

kureuil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kivikakk commented Oct 16, 2017

Uh oh!

kureuil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kivikakk commented Oct 16, 2017

Uh oh!

carols10cents commented Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kivikakk commented Oct 26, 2017

Uh oh!

kivikakk commented Oct 26, 2017

Uh oh!

kivikakk commented Oct 26, 2017

Uh oh!

kivikakk commented Oct 26, 2017

Uh oh!

carols10cents commented Oct 29, 2017

Uh oh!

bors-voyager bot commented Oct 29, 2017

Build succeeded

Uh oh!

Uh oh!

carols10cents commented Oct 23, 2017 •

edited

Loading