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

Use env::func(), not 'the function env::func' in docs for std::env #75945

Merged
merged 2 commits into from
Sep 1, 2020

Conversation

pickfire
Copy link
Contributor

Follow up of #75629

r? @jyn514

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 26, 2020
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

I do wonder whether this makes the docs a little harder to read ... it's idiomatic to use env::var and not var for example. @poliorcetics , what do you think?

library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
@jyn514 jyn514 added A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools labels Aug 26, 2020
@pickfire
Copy link
Contributor Author

@jyn514 Nice idea, I haven't thought about this since I was focused on clearing things up. Clear! Go, go, go...

@poliorcetics
Copy link
Contributor

I think env::something is usually clearer but here it's the documentation for the module itself so maybe it's ok ?

A tentative plus for env::something because being consistent in docs is important but I could be convinced of the opposite.

@pickfire
Copy link
Contributor Author

pickfire commented Aug 27, 2020

@poliorcetics I think it is better to write stuff that was in the module path so that it is easier to understand from users perspective, so the relative path is understandable there user already know the context when browsing the same page.

@poliorcetics
Copy link
Contributor

After looking at the docs I must say I personally prefer the env::item format but I'm not opposed to the item one either, especially when (like in the linked example) the phrasing makes it clear.

@jyn514
Copy link
Member

jyn514 commented Aug 27, 2020

@pickfire do you mind changing this to use env::var() instead of var()? The changes going from 'the env::var function' to env::var() are still helpful I think, but it's not clear that going from env::var() to var() is.

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2020
@pickfire
Copy link
Contributor Author

pickfire commented Aug 28, 2020

@pickfire do you mind changing this to use env::var() instead of var()? The changes going from 'the env::var function' to env::var() are still helpful I think, but it's not clear that going from env::var() to var() is.

Don't I need to add [`var`]: env::var? The main reason I add this is to reduce explicit links so it allows readers from the docs not to need to read the link in text form. Is intra-doc links able to figure out the current module to let us use the current module in front? Like env::var without having to put in explicit link? If yes then I do think that way is good but otherwise I think it results in a similar thing (my original reason is to make reading both html and text better).

@jyn514
Copy link
Member

jyn514 commented Aug 28, 2020

Other way around, you need to add [env::var]: var.

The main reason I add this is to reduce explicit links so it allows readers from the docs not to need to read the link in text form.

I think we should optimize for readers of the HTML first and of the markdown second. Adding the link is not a giant burden, there are already hundreds of links in libstd, and IMO it makes the HTML a lot more clear.

@pickfire
Copy link
Contributor Author

pickfire commented Aug 28, 2020

Other way around, you need to add [env::var]: var.

Why not add the module implicitly since most rust imports consists of xxx::yyy? I do find use xxx quite often, maybe we could do an implicit use xxx in rustdoc?

@jyn514
Copy link
Member

jyn514 commented Aug 28, 2020

Sounds similar to #59039, can you add a comment there? Although see also #73445 which is for crates and not modules.

@pickfire
Copy link
Contributor Author

Okay, we can leave it aside. @jyn514 how do you think we should proceed with this patch?

@jyn514
Copy link
Member

jyn514 commented Aug 28, 2020

I think if you change var to env::var and similar, it will be good to go. However I'm not comfortable merging it as-is.

library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
library/std/src/env.rs Outdated Show resolved Hide resolved
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Can you squash the commits? r=me with that done and nit addressed

library/std/src/env.rs Outdated Show resolved Hide resolved
@pickfire
Copy link
Contributor Author

pickfire commented Aug 30, 2020

How do I squash the commits on the web? I feel like opening another pull request is easier. Hehe

@jyn514
Copy link
Member

jyn514 commented Aug 30, 2020

I'm not aware that you can. I recommend using git locally: git rebase -i master and changing all the pick lines to squash except for the first.

@pickfire
Copy link
Contributor Author

pickfire commented Aug 30, 2020

I'm not aware that you can. I recommend using git locally: git rebase -i master and changing all the pick lines to squash except for the first.

I do know how do do it, just feel lazy to do since I need to delete the branch later on. Anyway, I did it already.

library/std/src/env.rs Outdated Show resolved Hide resolved
@jyn514
Copy link
Member

jyn514 commented Aug 30, 2020

Now you need to squash again 😆

vars() rather than vars function

Co-authored-by: Joshua Nelson <[email protected]>

Use [xxx()] rather than the [xxx] function

Co-authored-by: Joshua Nelson <[email protected]>

Env text representation of function intra-doc link

Suggested by @jyn514

Link join_paths in env doc for parity

Change xxx to env::xxx for lib env doc

Add link requsted by @jyn514

Fix doc build with same link

Co-authored-by: Joshua Nelson <[email protected]>

Fix missing intra-doc link

Fix added whitespace in doc

Co-authored-by: Joshua Nelson <[email protected]>

Add brackets for `join_paths`

Co-authored-by: Joshua Nelson <[email protected]>

Use unused link join_paths

Removed same link for join_paths

Co-authored-by: Joshua Nelson <[email protected]>

Remove unsed link join_paths
library/std/src/env.rs Outdated Show resolved Hide resolved
@jyn514
Copy link
Member

jyn514 commented Aug 31, 2020

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 31, 2020

📌 Commit 1d017eb has been approved by jyn514

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 31, 2020
@jyn514 jyn514 changed the title Env use shorter intra-doc links in path Use env::func(), not 'the function env::func' in docs for std::env Aug 31, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 1, 2020
Rollup of 12 pull requests

Successful merges:

 - rust-lang#75945 (Use `env::func()`, not 'the function env::func' in docs for std::env)
 - rust-lang#76002 (Fix `-Z instrument-coverage` on MSVC)
 - rust-lang#76003 (Adds two source span utility functions used in source-based coverage)
 - rust-lang#76059 (Clean up E0764)
 - rust-lang#76103 (Clean up E0769)
 - rust-lang#76139 (Make `cow_is_borrowed` methods const)
 - rust-lang#76154 (Fix rustdoc strings indentation)
 - rust-lang#76161 (Remove notrust in rustc_middle)
 - rust-lang#76163 (README: Adjust Linux and macOS support platform and architecture)
 - rust-lang#76166 (Make `StringReader` private)
 - rust-lang#76172 (Revert rust-lang#75463)
 - rust-lang#76178 (Update expect-test to 1.0)

Failed merges:

r? @ghost
@bors bors merged commit b675824 into rust-lang:master Sep 1, 2020
@pickfire pickfire deleted the patch-7 branch September 1, 2020 15:01
@cuviper cuviper added this to the 1.48.0 milestone Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants