Skip to content

Add print-only url to rustup doc, fix #1288 - #1324

Closed
amar-laksh wants to merge 4 commits into
rust-lang:masterfrom
amar-laksh:doc-features
Closed

amar-laksh wants to merge 4 commits into
rust-lang:masterfrom
amar-laksh:doc-features

Conversation

@amar-laksh

Copy link
Copy Markdown

Sorry for the earlier one, this is the complete commit.

@Mark-Simulacrum Mark-Simulacrum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think there's also a .swp. file and another file 3 that snuck into the PR on accident; could you remove those?

if m.is_present("print-only") {
let path = toolchain.doc_path(doc_url);
match path {
Ok(v) => return Ok(println!("file:/{}", v.to_str().unwrap())),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This probably wants to be v.display().

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Cleared the extra files, BTW v.display() for some reason doesn't provide the exact URL. So didn't change that part. Thanks.

@bors

bors commented Apr 13, 2018

Copy link
Copy Markdown
Contributor

☔ The latest upstream changes (presumably #1390) made this pull request unmergeable. Please resolve the merge conflicts.

if m.is_present("print-only") {
let path = toolchain.doc_path(doc_url);
match path {
Ok(v) => return Ok(println!("file:/{}", v.to_str().unwrap())),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will produce a URL like file:/C:\Foo\bar on windows, or possibly something even stranger, can you update this to work on windows, and also add a test for this command.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sure, I'll try to work it out.

@amar-laksh amar-laksh May 27, 2018 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

BTW, shouldn't the URL be just the URL with no platform-specific prefixes?
I mean just the 'C:\Foo\bar' or whatever the platform-specific URL is. @Diggsey ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That is probably fine - at least on windows the browser is smart enough to turn a path into a file:/// URL.

@amar-laksh amar-laksh closed this May 28, 2018
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.

4 participants