Skip to content

Avoid doctest errors #677

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

Closed
wants to merge 1 commit into from
Closed

Avoid doctest errors #677

wants to merge 1 commit into from

Conversation

kornelski
Copy link

It seems like proc macros don't work in doctests:

test src/lib.rs -  (line 167) ... FAILED
test src/lib.rs -  (line 154) ... FAILED
test src/lib.rs -  (line 141) ... FAILED

error[E0433]: failed to resolve: could not find main in async_std

--> src/lib.rs:146:14
  |
8 | #[async_std::main]
  |              ^^^^ could not find `main` in `async_std`

The problem is visible when running cargo test --all. Tested on Rust 1.40 and 1.42 nightly.

@yoshuawuyts
Copy link
Contributor

@kornelski in order to run the tests a few features need to be enabled:

$ cargo test --all --features 'attributes,unstable'

This is the way it's setup in our CI as well. Though as evidenced by this PR it's not discoverable, and we should at the very least document this in the README. I wish we could add this to profiles somehow, but I don't believe we can.

[profiles.test]
features = [ "attributes", "unstable" ]

@kornelski
Copy link
Author

kornelski commented Jan 15, 2020

That's unexpected indeed.

Doctests are an odd case, but for tests in general I expect them to be gated by the same flags as the features implementations are, so that cargo test only tests enabled features.

Maybe these doctests could be made conditional? This seems to be a supported syntax:

#[cfg_attr(feature = "attributes", doc = "#[async_std::main]")]

@kornelski
Copy link
Author

cfg_attr actually works!

Copy link
Contributor

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

Didn't know this was possible -- but this seems reasonable; thanks heaps!

@yoshuawuyts
Copy link
Contributor

Ah, it seems we're missing a cargo fmt pass. CI is failing. Would you mind updating?

@kornelski
Copy link
Author

kornelski commented Jan 15, 2020

Sorry, no.

rustfmt doesn't understand this syntax and breaks Markdown in the comments.

I'm incredibly disappointed that rustfmt missed the point of gofmt. It unconditionally overrides all human decisions, which makes it a dumb and destructive tool, unlike gofmt which more or less only cleans up known-bad formatting. Because of that I think unconditionally using rustfmt is a bad idea, and I'm not going to use rustfmt.

Copy link
Member

@k-nasa k-nasa left a comment

Choose a reason for hiding this comment

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

Thank you for a great hack!

Isn't this cause here?

format_code_in_doc_comments = true

I agree to remove rustfmt from CI if it gets in the way, but I think it would be better to remove the format_code_in_doc_comments option and keep using it.

What do you think ❓

@taiki-e
Copy link
Contributor

taiki-e commented Jan 16, 2020

Yeah, rustfmt error is bug/limitation of unstable format_code_in_doc_comments option.

Removing usage of format_code_in_doc_comments option should be the simplest fix, if we keep using rustfmt.
And I prefer to remove usage of this option for now because of the following reasons:

  • this option is unstable
  • (AFAIK) many docs with examples of this crate are in macros and rustfmt doesn't format code in macros, so are the benefits of using this option in this crate should not be very big.

fwiw: to keep using format_code_in_doc_comments, use cfg on hidden function instead of using #[doc] attribute, like:

/// ```
/// # #[cfg(feature = "attributes")]
/// # fn dox() {
/// examples...
/// # }
/// ```

@k-nasa
Copy link
Member

k-nasa commented Jan 28, 2020

ping @kornelski

@kornelski
Copy link
Author

Sorry, I don't like enforced rustfmt checks, and I'm not going to satify rustfmt's arbitrary requirements.

@kornelski kornelski closed this Jan 28, 2020
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.

4 participants