Open doc for specific topic #2019

dorfsmay · 2019-09-27T02:40:58Z

Feature to allow to open a specific doc eg:

rustup doc core::arch
rustup doc std::never
rustup doc println!
rustup doc fs::read_dir
rustup doc io::Bytes
etc...

This replicates the way pydoc works. I have been using this feature a lot while writing it to the point that I really missed it when deploying a broken version!

This is not 100% finished, I'd like some feedback and understand the appetite for it before putting more time into it. However, I think it is functional enough to be merged as is. Two pieces I want to add:

add integration tests. This feature requires all the html files to be installed to work, so unit tests cannot be written for it. I'm not where in the module I could add integration tests.
I want to add a way to select primitive vs module when there is a conflict. Currently rustup doc usize will open the browser to the page for the module "usize", it would be nice to have a way to open the page for the primitive. I'm thinking either add an option like -p, or append say a "+" sign at the end of the word (e.g.: usize+) to force the doc for the primitive. I personally prefer the latter, but am looking for feedback...

Built and tested on both Rust 1.37 and 1.38.

dorfsmay · 2019-09-27T04:59:34Z

The fail build is due to clippy errors from master on file that this PR is not touching.

kinnison · 2019-09-27T06:47:10Z

Regarding the failed build, assuming #2023 passes, I'll merge a fix for the clippy lint issues and you can rebase -- however you do have some formatting problems which I will leave for you.

Regarding the idea in general, I quite like it, though I wonder if this ought to depend on --std being passed perhaps? Perhaps not, because I was thinking that you could disambiguate between usize the primitive and usize the module, by if the user says rustup doc usize that's the primitive, but rustup doc std::usize is the module.

dorfsmay · 2019-09-27T18:33:29Z

I have corrected the formatting issues.

I think adding std:: to specify module instead primitive has the disadvantage of to having to add std:: to almost everything. I'm wondering if adding primitive::usize or .::usize, or even just ::usize for primite, would result in a better experience overall.

kinnison · 2019-09-27T21:01:05Z

So my feeling is that we can satisfy both the desire for shorter names, and also using std::prim to distinguish between a primitive and the module as follows:

If there is no :: present, try for a primitive
Failing either of those, assume it's a path suffix.

dorfsmay · 2019-09-28T05:13:41Z

Thought more about it, and reverted to your original suggestion: For anything in a module or bellow, it requires std:: (or core::), a single word will be matched against primitives, keywords and macros.

This is follows Rust's own standard, we have to specify std:: when specifying a module with use, and it does not push us in corners we might regret later. I have updated the code to reflect this change.

kinnison · 2019-09-28T05:43:07Z

@dorfsmay Okay that sounds good, I'll give this a go and then think about your original question about how we might test it.

kinnison · 2019-09-28T05:44:37Z

While I think about that, if you have time, could you rebase/clean up your commit history a bit -- I'd prefer to have nice functional clean commits while reviewing, rather than dealing with formatting commits etc.

If you're unsure how to do that, I am prepared to do the trivial option of simply rebasing everything together into one commit for you if you want.

dorfsmay · 2019-09-28T05:49:27Z

What are the alternatives to rebasing to a single commit?

I'm open... Just trying to understand what the options are, and their pros and cons.

kinnison · 2019-09-28T05:52:58Z

If I'm doing it for you, I'd do a single commit because it's easiest for me, if you do it, you can clean it up into commits which are nice and clean in the sense that one commit does one thing. I'd rather see a logical progression of functionality rather than a progression of your work if you see what I mean.

dorfsmay · 2019-09-28T06:01:44Z

There really is a single functionality, the different commits are just my steps to get there, going back and forth between ideas discussed here, formatting etc...

Feel free to squash them into a single commit if you have the time, otherwise I'll try to get to it tomorrow or Sunday. Thanks.

kinnison · 2019-09-28T06:16:28Z

I like this concept, but the complexity of finding things in the documentation isn't easy to deal with. not least, as a user I'd expect rustup doc Iterator to show me std::iter::Iterator given the feel you're going for.

I suggest that it might be simpler (and more orthogonal) to simply take the arguments and pass them in as ?search=ARGS to the index.html of the documentation selected (displaying an error if the user expresses a search term without specifying a doc type to open

What do you think of that?

i.e.

rustup doc -> opens the doc index
rustup doc Iterator -> an error (or optionally we could default to --std in that context)
rustup doc --somebook query terms -> .../somebook/index.html?search=query+terms
rustup doc --std Iterator would then work, as would rustup doc --std std::io::Write though the latter looks odd, repeating std hence perhaps rustup doc std::io::Write would be treated as an implicit --std

What do you think?

dorfsmay · 2019-09-28T16:29:48Z

I had completely missed the fact that there was a search function on those pages! That's really neat, thanks!

I like your idea here, but, I am not finding any way to open a browser to a local file with a query parameter. It works fine with an online URL (http://, https://), but not with a local one. The browsers transform the ? into %3F and try, and fail, to open that particular file (e.g.: …/std/index.html%3Fsearch=iterator). I've tried both with firefox and chromium-browser on Linux.

kinnison · 2019-09-28T17:03:34Z

Hmm, I'd tested firefox by firefox "file:///...../index.html?search=Iterator" and that worked. The xdg-open tool doesn't seem to support it though, so perhaps that idea is moot :(

dorfsmay · 2019-09-28T20:20:29Z

Squashed into a single commit.

kinnison

There's at least alloc to think of, and also the use of OsString where PathBuf may be more appropriate. Then we need to finally decide what we're going to do about testing (if anything beyond by-hand on the tier 1 platforms)

kinnison · 2019-09-29T06:30:24Z

src/cli/doc_subpath.rs

+fn index_html(doc: &DocData, wpath: &Path) -> Option<OsString> {
+    let indexhtml = wpath.join("index.html");
+    match &doc.root.join(&indexhtml).exists() {
+        true => Some(indexhtml.into_os_string()),


Any reason you don't make this function return a PathBuf and save the conversions ?

kinnison · 2019-09-29T06:31:37Z

src/cli/doc_subpath.rs

+        for k in keywords {
+            let filename = &format!("{}.{}.html", k, doc.subtopic);
+            if entries.contains(&OsString::from(filename)) {
+                return Ok(dir.join(filename).into_os_string());


Again here, return Result<PathBuf> and this conversion goes away at least.

kinnison · 2019-09-29T06:33:46Z

src/cli/doc_subpath.rs

+    // topic.split.count cannot be 0
+    let subpath_os_str = match topic_vec.len() {
+        1 => match topic {
+            "std" | "core" => match index_html(&doc, &work_path) {


I think alloc is also plausibly something for here?

src/cli/doc_subpath.rs

dorfsmay · 2019-09-30T15:06:58Z

I have:

added "alloc". Thanks for pointing it out.
removed the OsString: It made sense to use it when I started this, but not any more, a single conversion right at the end is much cleaner
I have added logic to be able to search for macros etc... outside of std:
- rustup doc usize → std/primitive.usize.html
- rustup doc std::usize → std/usize/index.html`
- rustup doc alloc::format! → alloc/macro.format!.html

Let me know what you think, or if I've missed other cases. I'll start working on a test later today.

kinnison · 2019-10-02T19:38:00Z

I think that looks pretty good, though it'd be handy if we didn't need the ! for the macro, since that's a pain to put in a UNIX shell. I'm fine if the first version needs it though. I look forward to tests.

dorfsmay · 2019-10-06T04:57:05Z

Two things regarding the macros:

looking for a macro with or without an exclamation mark works
exclamation mark at the end of a word shouldn't be an issue in bash - I tested rustup doc println! on in bash on Ubuntu, and it works fine (I don't know about zsh, fish etc...).

I have finally added tests. After trying different ideas it looks like the simplest solution is to mock the cases we need to test the same way the default index.html is mocked. I use a separate file in order to be able to use when creating the mock artifacts and and when testing the code behaviour. I tried to keep the format of that file as simple as possible since it will potentially be updated by humans when adding or changing tests.

kinnison

Two tweaks needed and then I think this might be ready to merge.

kinnison · 2019-10-06T07:27:26Z

src/cli/topical_doc.rs

+    };
+    // The path and filename were validated to be existing on the filesystem.
+    // It should be safe to unwrap, or worth panicking.
+    Ok(subpath_os_path.into_os_string().into_string().unwrap())


I'd still be tempted to say you should be returning Result<PathBuf> so that you don't need to do these conversions.

kinnison · 2019-10-06T07:30:59Z

src/cli/topical_doc.rs

+    };
+
+    /**************************
+


It may be worth referencing your tests/mock/topical_doc_data.rs file here to say that if additional functionality is added, it should be added to the tests so that it can be verified. That way we'll keep support for new doc sections properly supported if std is split up further.

dorfsmay · 2019-10-06T16:51:48Z

I have added a line in the comment as suggested regarding the test.

I also moved the conversion from PathBuf to &str to rustup_mode.rs.

kinnison

This looks good. I want to have a bit of experimentation to confirm the UX is good, but my gut feeling is that I'll merge it soon (before the next release)

kinnison · 2019-10-08T11:43:22Z

My local testing suggests this is good for MVP, I'm going to merge.

kinnison reviewed Sep 29, 2019

View reviewed changes

kinnison suggested changes Oct 6, 2019

View reviewed changes

Adding ability to open doc on a specific topic

c50626e

dorfsmay requested a review from kinnison October 6, 2019 16:50

kinnison approved these changes Oct 6, 2019

View reviewed changes

kinnison merged commit b9e968b into rust-lang:master Oct 8, 2019

dorfsmay deleted the open_doc_for_topic branch October 10, 2019 04:22

Open doc for specific topic #2019

Open doc for specific topic #2019

Uh oh!

Conversation

dorfsmay commented Sep 27, 2019

Uh oh!

dorfsmay commented Sep 27, 2019

Uh oh!

kinnison commented Sep 27, 2019

Uh oh!

dorfsmay commented Sep 27, 2019

Uh oh!

kinnison commented Sep 27, 2019

Uh oh!

dorfsmay commented Sep 28, 2019

Uh oh!

kinnison commented Sep 28, 2019

Uh oh!

kinnison commented Sep 28, 2019

Uh oh!

dorfsmay commented Sep 28, 2019

Uh oh!

kinnison commented Sep 28, 2019

Uh oh!

dorfsmay commented Sep 28, 2019

Uh oh!

kinnison commented Sep 28, 2019

Uh oh!

dorfsmay commented Sep 28, 2019

Uh oh!

kinnison commented Sep 28, 2019

Uh oh!

dorfsmay commented Sep 28, 2019

Uh oh!

kinnison left a comment

Choose a reason for hiding this comment

Uh oh!

kinnison Sep 29, 2019

Choose a reason for hiding this comment

Uh oh!

kinnison Sep 29, 2019

Choose a reason for hiding this comment

Uh oh!

kinnison Sep 29, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dorfsmay commented Sep 30, 2019

Uh oh!

kinnison commented Oct 2, 2019

Uh oh!

dorfsmay commented Oct 6, 2019

Uh oh!

kinnison left a comment

Choose a reason for hiding this comment

Uh oh!

kinnison Oct 6, 2019

Choose a reason for hiding this comment

Uh oh!

kinnison Oct 6, 2019

Choose a reason for hiding this comment

Uh oh!

dorfsmay commented Oct 6, 2019

Uh oh!

kinnison left a comment

Choose a reason for hiding this comment

Uh oh!

kinnison commented Oct 8, 2019

Uh oh!

Uh oh!