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

Cleanup all doc warnings #806

Merged
merged 2 commits into from
Dec 11, 2017
Merged

Cleanup all doc warnings #806

merged 2 commits into from
Dec 11, 2017

Conversation

Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Dec 2, 2017

With the impending switch to Pulldown as the default doc generator, warnings
have been enabled for incompatible syntax. This fixes all of said warnings.

@asomers
Copy link
Member

asomers commented Dec 3, 2017

What switch to Pulldown? Is this a new feature of rustc?

@Susurrus
Copy link
Contributor Author

Susurrus commented Dec 3, 2017

rustdoc is switching from whatever it's using now to pulldown-cmark. This is good because it moves us to use CommonMark, which is standardized, than whatever the previous thing was doing. And it's a Rust-only lib. But there are some syntax changes. There's been warnings for this on nightly for a while and I believe Rust 1.22 includes these warnings as well.

@Susurrus
Copy link
Contributor Author

Susurrus commented Dec 4, 2017

@asomers For reference here's the tracking bug: rust-lang/rust#38400

And as of 1.18, you can enable the new rendering with

@Susurrus
Copy link
Contributor Author

Susurrus commented Dec 9, 2017

@asomers want to give this a review or are you fine with me merging it as-is?

@@ -1124,12 +1125,24 @@ pub fn getgroups() -> Result<Vec<Gid>> {
/// specific user and group. For example, given the user `www-data` with UID
/// `33` and the group `backup` with the GID `34`, one could switch the user as
/// follows:
/// ```
///
/// ```rust,no_run
Copy link
Member

Choose a reason for hiding this comment

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

Why are you adding no_run ? If this was an unreliable doc test, then you should fix it in a separate commit than the commit that fixes the doc warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was never run before, that's the biggest part of this change. There were doctests that weren't being found and executed, so this doctest has never been run. And turns out it fails when run.

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 can put both of these changes in a separate commit that explains all this.

Copy link
Member

Choose a reason for hiding this comment

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

Do you understand why they weren't being found? Because I don't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous parser didn't find them, so they weren't run. I was always curious why, but I never cared enough to actually read up on it. If you ran our tests right now with the latest Rust, it'd explain this in the compiler warnings, though not give too many specifics.

Bryant Mairs added 2 commits December 10, 2017 19:20
The switch to Pulldown for the Markdown parser for Rustdoc has revealed
that there were a large number of doctests that weren't being run. These
tests failed to compile before, but will also fail to run because they
attach to arbitrary user and group IDs, which likely don't exist on the
system. These are therefore marked as `no_run`.
With the impending switch to Pulldown as the default doc generator, warnings
have been enabled for incompatible syntax. This fixes all of said warnings.
@asomers
Copy link
Member

asomers commented Dec 11, 2017

Ok, perhaps a parser bug or something. Probably not worth investigating further.

bors r+

bors bot added a commit that referenced this pull request Dec 11, 2017
806: Cleanup all doc warnings r=asomers a=Susurrus

With the impending switch to Pulldown as the default doc generator, warnings
have been enabled for incompatible syntax. This fixes all of said warnings.
@bors
Copy link
Contributor

bors bot commented Dec 11, 2017

@bors bors bot merged commit 133e350 into nix-rust:master Dec 11, 2017
@Susurrus Susurrus deleted the doc_warnings branch December 11, 2017 04:49
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.

2 participants