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

Reenable linkchecker for books #39976

Merged
merged 3 commits into from
Feb 20, 2017

Conversation

steveklabnik
Copy link
Member

@steveklabnik steveklabnik commented Feb 20, 2017

In some senses, this is a revert of cacb3bc#diff-b64563d143f859565c8357a28ef81101R212; we disabled linkchecker for the book because the links were added by JavaScript. Now, that's fixed upstream, and so we can re-enable the checker.

This also involves two other fixes: we have to check for names as well as ids for links, and the linking algorithm of mdBook changed to the same as rustdoc's, so we change some links back.


This is good to go 😄 

This reverts commit 7f1d1c6.

The original commit was created because mdBook and rustdoc had
different generation algorithms for header links; now with
rust-lang#39966 , the algorithms
are the same. So let's undo this change.

... when I came across this problem, I said "eh, this isn't fun,
but it doesn't take that long." I probably should have just actually
taken the time to fix upstream, given that they were amenable. Oh
well!
@rust-highfive
Copy link
Collaborator

@steveklabnik: no appropriate reviewer found, use r? to override

@steveklabnik
Copy link
Member Author

r? @frewsxcv

@frewsxcv
Copy link
Member

Travis:

error: the lock file needs to be updated but --frozen was passed to prevent this

make: *** [tidy] Error 101

@frewsxcv
Copy link
Member

r=me with lockfile thing sorted out

@steveklabnik
Copy link
Member Author

steveklabnik commented Feb 20, 2017

hrm, I don't understand why it's doing that; tidy passes for me locally.

EDIT: Oh, I forgot to commit the lockfile, dunno why...

@steveklabnik steveklabnik force-pushed the reenable-book-linkchecker branch from 5a42c59 to f5951af Compare February 20, 2017 16:03
@steveklabnik
Copy link
Member Author

@bors: r=frewsxcv

@bors
Copy link
Contributor

bors commented Feb 20, 2017

📌 Commit f5951af has been approved by frewsxcv

Previously, mdBook used JavaScript to add header links, so we
skipped checking the book. As of
rust-lang#39966, it no longer does,
so we can start checking again.

There is a twist, though: it uses name instead of id, so let's test
for both. They're both valid links anyway, so it's good to have the
checker check anyway.
This brings in a needed bugfix.
@steveklabnik steveklabnik force-pushed the reenable-book-linkchecker branch from f5951af to 010a28d Compare February 20, 2017 16:18
@steveklabnik
Copy link
Member Author

@bors: r=frewsxcv

(I left in a warning by accident)

@bors
Copy link
Contributor

bors commented Feb 20, 2017

📌 Commit 010a28d has been approved by frewsxcv

@ollie27
Copy link
Member

ollie27 commented Feb 20, 2017

Why have you used the name attribute?

@steveklabnik
Copy link
Member Author

Why have you used the name attribute?

That is what mdbook has always used for these links.

@ollie27
Copy link
Member

ollie27 commented Feb 20, 2017

Is there a good reason for that? I'm pretty sure id is the correct thing to use.

@steveklabnik
Copy link
Member Author

Is there a good reason for that? I'm pretty sure id is the correct thing to use.

You should file a bug on mdbook about it.

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 20, 2017
…ker, r=frewsxcv

Reenable linkchecker for books

In some senses, this is a revert of rust-lang@cacb3bc#diff-b64563d143f859565c8357a28ef81101R212; we disabled linkchecker for the book because the links were added by JavaScript. Now, that's fixed upstream, and so we can re-enable the checker.

This also involves two other fixes: we have to check for `name`s as well as `id`s for links, and the linking algorithm of mdBook changed to the same as rustdoc's, so we change some links back.

~~~This isn't quite ready yet; it's [depending on a PR of mine to mdBook](https://github.com/azerupi/mdBook/pull/209). After that's released, this should be the last of these kinds of shenanigans~~~ 😄

This is good to go 😄
bors added a commit that referenced this pull request Feb 20, 2017
Rollup of 3 pull requests

- Successful merges: #39913, #39937, #39976
- Failed merges:
@bors bors merged commit 010a28d into rust-lang:master Feb 20, 2017
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.

5 participants