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

Point "deref coercions" links to new book #43631

Merged
merged 3 commits into from
Aug 23, 2017
Merged

Conversation

ruuda
Copy link
Contributor

@ruuda ruuda commented Aug 3, 2017

Currently the link on doc.rust-lang.org is semi-broken; it links to a page that links to the exact page in the first edition in the book, or to the index of the second edition of the book. If the second editions
is the recommended one now, we should point the links at that one. (In the mean time, the links have been updated to point directly to the first edition of the book, but that hasn't made it onto
the stable channel yet.) By the time this commit makes it onto the stable channel, the second edition of the book should be complete enough. At least the part about deref coercions is.

r? @steveklabnik

@ruuda
Copy link
Contributor Author

ruuda commented Aug 3, 2017

It does generate tidy errors:

tidy error: rust/src/libcore/ops/deref.rs:16: line longer than 100 chars
tidy error: rust/src/libcore/ops/deref.rs:74: line longer than 100 chars
tidy error: rust/src/libstd/lib.rs:206: line longer than 100 chars

Is there anything that can be done about that?

@aidanhs aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 3, 2017
@aidanhs
Copy link
Member

aidanhs commented Aug 3, 2017

@ruuda thanks for the PR! We'll check in now and again to make sure @steveklabnik or another reviewer gets to it soon.

There are allowances made for URLs (src/tools/tidy/src/style.rs, long_line_is_ok), but I guess it doesn't pick up relative paths so I suppose that would need to be fixed as well. I don't know if there's already an issue open, but I think it'd just mean altering w.starts_with("http://") || w.starts_with("https://") to also allow for ../.

@ruuda
Copy link
Contributor Author

ruuda commented Aug 3, 2017

@aidanhs Thank you for pointing me in the right direction. I’ve opened #43632. With that applied, this PR now passes the tidy check.

//! calls to methods on [`str`] and [`[T]`][slice] respectively, via [deref
//! coercions].
//! calls to methods on [`str`] and [`[T]`][slice] respectively, via
//! [deref-coercions].
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? (Hyphen is added to the link text.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it because the tidy check does not deal with spaces in named links, not realising that it was the short form. Fixed.

@arielb1
Copy link
Contributor

arielb1 commented Aug 8, 2017

Tidy errors: (you can rebase your PR on top of master to ignore them?)

This is waiting for #43632

@arielb1 arielb1 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 8, 2017
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 11, 2017
…ark-Simulacrum

Detect relative urls in tidy check

This came up in rust-lang#43631: there can be long relative urls in Markdown comments, that do not start with `http://` or `https://`, so the tidy check will not detect them as urls and complain about the line length. This PR adds detection of relative urls starting with `../`.
MaloJaffre added a commit to MaloJaffre/rust that referenced this pull request Aug 11, 2017
…ark-Simulacrum

Detect relative urls in tidy check

This came up in rust-lang#43631: there can be long relative urls in Markdown comments, that do not start with `http://` or `https://`, so the tidy check will not detect them as urls and complain about the line length. This PR adds detection of relative urls starting with `../`.
@ruuda
Copy link
Contributor Author

ruuda commented Aug 11, 2017

@arielb1: Rebased, this now passes the tidy check.

@bors
Copy link
Contributor

bors commented Aug 12, 2017

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

ruuda added 3 commits August 13, 2017 11:20
Currently the link on doc.rust-lang.org is semi-broken; it links to a
page that links to the exact page in the first edition in the book, or
to the index of the second edition of the book. If the second editions
is the recommended one now, we should point the links at that one. It
seems that in the mean time, the links have been updated to point
directly to the first edition of the book, but that hasn't made it onto
the stable channel yet. By the time this commit makes it onto the stable
channel, the second edition of the book should be complete enough. At
least the part about deref coercions is.
With the space, the tidy check does not recognize it as
a link label. See also the comment above in line_is_url()
in src/tools/tidy/src/style.rs.
The text does not need the hyphen, but the anchor name does.
@arielb1
Copy link
Contributor

arielb1 commented Aug 15, 2017

Hi @steveklabnik - I think this is ready for review.

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

arielb1 commented Aug 22, 2017

r? @QuietMisdreavus

@QuietMisdreavus
Copy link
Member

@bors r+ rollup

Thanks! (oops, i misspelled the bot's name in my last one >_>)

@bors
Copy link
Contributor

bors commented Aug 22, 2017

📌 Commit e63d979 has been approved by QuietMisdreavus

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Aug 23, 2017
Point "deref coercions" links to new book

Currently the link on doc.rust-lang.org is semi-broken; it links to a page that links to the exact page in the first edition in the book, or to the index of the second edition of the book. If the second editions
is the recommended one now, we should point the links at that one. (In the mean time, the links have been updated to point directly to the first edition of the book, but that hasn't made it onto
the stable channel yet.) By the time this commit makes it onto the stable channel, the second edition of the book should be complete enough. At least the part about deref coercions is.

r? @steveklabnik
bors added a commit that referenced this pull request Aug 23, 2017
Rollup of 8 pull requests

- Successful merges: #43631, #43977, #43983, #44016, #44039, #44043, #44047, #44054
- Failed merges:
@bors bors merged commit e63d979 into rust-lang:master Aug 23, 2017
@ruuda ruuda deleted the update-docs branch August 23, 2017 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants