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

rustdoc: remove the ! from macro URLs and titles #35234

Merged
merged 3 commits into from
Aug 20, 2016

Conversation

nrc
Copy link
Member

@nrc nrc commented Aug 3, 2016

Because the ! is part of a macro use, not the macro's name. E.g., you write macro_rules! foo not macro_rules! foo!, also #[macro_import(foo)].

(Pulled out of #35020).

@nrc nrc added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Aug 3, 2016
@rust-highfive
Copy link
Collaborator

r? @steveklabnik

(rust_highfive has picked a reviewer for you, use r? to override)

@nrc
Copy link
Member Author

nrc commented Aug 3, 2016

cc @alexcrichton

@alexcrichton
Copy link
Member

Sounds good to me! @steveklabnik what do you think?

@ollie27
Copy link
Member

ollie27 commented Aug 4, 2016

I don't think we can change the URLs because it will break links.

We could change the titles but it brings up an interesting point about how we refer to macros. For instance on the page for std::fmt almost every reference to a macro includes the !.

@nrc
Copy link
Member Author

nrc commented Aug 4, 2016

I don't mind breaking a few links, I don't think it will be a huge impact. I'd be fine with adding redirects, but with no warnings, that is just pushing the breakage down the road, unless we want to leave the redirects forever (which I guess is not too bad).

@steveklabnik
Copy link
Member

I like the idea, but also share concern about breaking things.

Across all these PRs, I have no idea what the "right" thing is; at some point, we will have to break stuff. It'd be kinda nice to do it all at once, though...

@nrc
Copy link
Member Author

nrc commented Aug 4, 2016

See also rust-lang/rfcs#1561, which has a bunch of discussion in the comment thread about whether ! is part of the macro name or usage.

@nrc
Copy link
Member Author

nrc commented Aug 5, 2016

@steveklabnik this is my only PR with a breaking change in it at the moment, I don't have anything else planned and I think the bustage here will be pretty minor. I do think we should be willing to eat some breakage in Rustdoc - the implementation needs some serious work and unfortunately what should be implementation details is exposed in public interfaces such as URLs, if we're forever shy of breaking, we'll be stuck with the current implementation. Agree it's good to have a strategy to minimise pain.

@ollie27
Copy link
Member

ollie27 commented Aug 5, 2016

Well if you do change the URLs, for this to pass linkchecker you'll need to add redirects and/or update all the manual links in the docs.

@steveklabnik
Copy link
Member

Gonna add this to the doc team meeting agenda for this week

@nrc
Copy link
Member Author

nrc commented Aug 8, 2016

@steveklabnik let me know what you come up with - I can update the links in the docs, but not worth doing unless you'll take the patch

@steveklabnik
Copy link
Member

@nrc We talked at the doc team, and we're 👍 on merging this patch. Please do!

@alexcrichton
Copy link
Member

@bors: r+ b55a28317fcb1c5e175d2b1dcba1b6088a808231

May as well help to see the errors from the linkchecker!

@bors
Copy link
Contributor

bors commented Aug 12, 2016

⌛ Testing commit b55a283 with merge 7097fe5...

@bors
Copy link
Contributor

bors commented Aug 12, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@cjsut
Copy link
Contributor

cjsut commented Aug 14, 2016

This will break any links to macros in user-crate documentation.
Does crater check for broken links? Hell, does rustdoc even check for broken links? I find it usually just renders the markdown as text instead of producing an error!

@nrc
Copy link
Member Author

nrc commented Aug 14, 2016

Crater does not. Rustdoc does not. There is a linkchecker tool which does.

This will be a breaking change for docs.

@ollie27
Copy link
Member

ollie27 commented Aug 15, 2016

We don't have to break anything if we just add redirects. IMO if we change URLs then we have to add redirects.

@steveklabnik
Copy link
Member

Yes, we want to make sure the redirects are here.

@nrc
Copy link
Member Author

nrc commented Aug 16, 2016

Now with links fixed and redirects added (no longer a breaking change).

r? @steveklabnik

@ollie27
Copy link
Member

ollie27 commented Aug 16, 2016

Maybe add a test to make sure the redirects are generated correctly?

@nrc
Copy link
Member Author

nrc commented Aug 16, 2016

now with bonus test

@bors
Copy link
Contributor

bors commented Aug 17, 2016

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

@nrc
Copy link
Member Author

nrc commented Aug 18, 2016

rebased

@nrc
Copy link
Member Author

nrc commented Aug 19, 2016

ping @steveklabnik r?

@steveklabnik
Copy link
Member

@bor: r+ rollup

@ollie27
Copy link
Member

ollie27 commented Aug 19, 2016

@steveklabnik looks like you misspelt @bors

@alexcrichton
Copy link
Member

@bors: r=steveklabnik rollup

@bors
Copy link
Contributor

bors commented Aug 19, 2016

📌 Commit 301401e has been approved by steveklabnik

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Aug 20, 2016
rustdoc: remove the `!` from macro URLs and titles

Because the `!` is part of a macro use, not the macro's name. E.g., you write `macro_rules! foo` not `macro_rules! foo!`, also `#[macro_import(foo)]`.

(Pulled out of rust-lang#35020).
sophiajt pushed a commit to sophiajt/rust that referenced this pull request Aug 20, 2016
rustdoc: remove the `!` from macro URLs and titles

Because the `!` is part of a macro use, not the macro's name. E.g., you write `macro_rules! foo` not `macro_rules! foo!`, also `#[macro_import(foo)]`.

(Pulled out of rust-lang#35020).
bors added a commit that referenced this pull request Aug 20, 2016
@bors bors merged commit 301401e into rust-lang:master Aug 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants