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

Allow rustup doc books open subsections and paragraphs #3380

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

YegorStolyarov
Copy link

@YegorStolyarov YegorStolyarov commented Jun 8, 2023

Reason: additional param to doc command, topic, is only supported for rust reference. I tried to implement minimalistic changes to make it work with books as well.
based on this issue #3345

What does it do: exact links to offline docs in books.
for example, rustup doc --rustc platform-support/armv6k-nintendo-3ds.html#building-the-target now opens exact chapter and paragraph in rustc book

Code changes:

  • added logic case for condition (book flag) AND (topic param provided) . now topic param is ignored in case of opening book
  • have to add "file:///" to browser url because opener crate which used in rustup have problems to recognize url as file when it contains "#" symbol.

my first PR, thank you for the feedback.

Egor Stolyarov added 2 commits June 8, 2023 16:04
now additional param to doc command, topic, is only supported for rust reference.
I tried to implement minimalistic changes to make it work with books as well
Copy link
Member

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

@rbtcollins Do you have any thoughts about adding this feature?

} else {
"index.html"
};
let cached_path: String;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to define this one here? Maybe we can put it into the if statement.

Copy link
Author

@YegorStolyarov YegorStolyarov Jun 9, 2023

Choose a reason for hiding this comment

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

This is the way to resolve &str borrowing issue. If we put this definition into if statement we will get error:
error[E0597]: cached_path does not live long enough. because source String must live as long as let doc_url lives.
TBH I just used the same workaround that is already used with let topical_path: PathBuf; one line above.
maybe there is a nicer way to solve this.


if m.get_flag("path") {
let doc_path = toolchain.doc_path(doc_url)?;
writeln!(process().stdout().lock(), "{}", doc_path.display())?;
Ok(utils::ExitCode(0))
} else {
toolchain.open_docs(doc_url)?;
if has_docs_data_link {
let url_path_buf = toolchain.doc_path(doc_url)?;
Copy link
Member

Choose a reason for hiding this comment

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

I think we can put it into open_docs. We don't need to leak it here. And the overall logic is very similar with the open_docs.

Copy link
Author

@YegorStolyarov YegorStolyarov Jun 9, 2023

Choose a reason for hiding this comment

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

I have tried to use open_docs(), but there was a problem with differentiating web links from file links when we use urls with anchors. Problem is caused by "#" symbol in url and located in the opener crate

So I had to use utils::open_browser() to prepend "file:///" to the url arbitrary, after generating url with toolchain.doc_path()

Or maybe we should go further and prepend "file:///" everywhere?

@rbtcollins
Copy link
Contributor

I like the concept of the feature.

I think we should see how it interacts:

  • using fragments with rustup doc ... on
    • mac
    • windows
    • linux
    • wsl

If the fragment doesn't get consistently forwarded to the browser then it may just cause more confusion.

I'd also love to see more testing around doc TBH. We have a very good test coverage in most of rustup, but some bits are not as strong ... I think here an appropriate testing strategy would be to do a unit test in the same file, checking for the emitted url for a few combinations of parameters. It might help to break out a helper function that takes the Option<>'s from clap and returns the url.

@YegorStolyarov
Copy link
Author

YegorStolyarov commented Jun 19, 2023

Currently I'm stuck with writing tests for my feature.
It's difficult for me to reproduce needed context using existing test helpers, or I am doing it wrong.

I tried to move url generating logic into separate function:

fn get_doc_url(m: &ArgMatches, t: &Toolchain<'_>) -> Result<(String, bool)> {
    if let Some((short, _, path)) = DOCS_DATA.iter().find(|(name, _, _)| m.get_flag(name)) {
        if let Some(topic) = m.get_one::<String>("topic") {
            let cached_path = format!("{}/{}", short, topic);
            Ok((cached_path, true))
        } else {
            Ok((path.to_string(), false))
        }
    } else if let Some(topic) = m.get_one::<String>("topic") {
        let topical_path = topical_doc::local_path(&t.doc_path("").unwrap(), topic)?;
        Ok((topical_path.to_str().unwrap().to_string(), false))
    } else {
        Ok(("index.html".to_string(), false))
    }
}

So it needs a mock toolchain for test.

Need some clues or an advice.

@Rustin170506
Copy link
Member

I tried to move url generating logic into separate function:

If you moved it into a separate function, then you can write some unit tests to cover it.

@rbtcollins
Copy link
Contributor

Toolchain takes a Cfg, a LocalToolchainName and a path (to the toolchain root dir).

Cfg has a pretty shallow needs for testing. See this example. We could of course make it better.

LocalToolchainName is a very narrow object with no exterior state, you can just create the one you want. E.g. LocalToolchainName::from(ToolchainDesc::from_str("stable").unwrap()).unwrap();

The path - since this is a unit test and meant to be in a read-only code path, you could just supply any arbitrary string you want quite sensibly I think.

test::mock::topical_doc_data might be useful, or at least relevant.

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.

3 participants