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

Disable needless_main_doctest by default #4858

Closed
gnzlbg opened this issue Nov 28, 2019 · 4 comments · Fixed by #4866
Closed

Disable needless_main_doctest by default #4858

gnzlbg opened this issue Nov 28, 2019 · 4 comments · Fixed by #4866

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 28, 2019

This lint is recommending me to remove the fn main() in jemalloc-ctl doctests, e.g., see these ones at the top-level: https://docs.rs/jemalloc-ctl/0.3.3/jemalloc_ctl/#examples

I personally think that the fn main() significantly increases the clarity of these particular doctests.

Also, one does not have to write fn main(), one explicitly must opt into writing it. Warning by default when one has explicitly opted into doing so feels like a bad idea.

So IMO this lint should not even be enabled when clippy::pedantic is enabled.

Looking at the lint PR, I can't find an issue or a discussion about the "need" for this lint. Which crate authors were writing fn main() in doctests out of ignorance that it could be left out, to the point that doing so warrants a lint ? The book and rust-by-example all omit fn main, so I'm skeptical that anybody learning Rust from any official resource would have picked up the habit of writing fn main() in doctest "by accident". In my experience, one has to go deep into the rustdoc documentation to figure out that this is even possible.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 28, 2019

cc @llogiq

@flip1995
Copy link
Member

A fix for this (only empty fn main() {}s get linted) is in #4729. But some weird errors occur because of the syn dep.

@llogiq
Copy link
Contributor

llogiq commented Nov 29, 2019

I must add that this is not a complete fix. However, once I have the syn dependency sorted out, I can omit linting when I encounter e.g. a static item.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 29, 2019

Could we maybe temporarily disable the lint until this is fixed ?

@bors bors closed this as completed in e47ae20 Dec 1, 2019
@bors bors closed this as completed in #4866 Dec 1, 2019
ilammy added a commit to ilammy/themis that referenced this issue Dec 23, 2019
Recently released Clippy 1.40 ships with this lint enabled by default.
Since we run with "warnings as errors" policy, this lint causes build
failures which is not nice.

    error: needless `fn main` in doctest
      --> src/wrappers/themis/rust/libthemis-src/src/lib.rs:37:4
       |
    37 | //! fn main() {
       |    ^^^^^^^^^^^^
       |
       = note: `-D clippy::needless-doctest-main` implied by `-D warnings`
       = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_doctest_main

    error: aborting due to previous error

I wouldn't say that explicit main() here is needless and some people
share the same sentiment [1]. However, since this is top-level module
docs, we cannot disable this lint for this place specifically without
turning it off in the entire module. Okay, Clippy, you win.

[1]: rust-lang/rust-clippy#4858
ilammy added a commit to cossacklabs/themis that referenced this issue Dec 23, 2019
* Fix clippy::needless-doctest-main

Recently released Clippy 1.40 ships with this lint enabled by default.
Since we run with "warnings as errors" policy, this lint causes build
failures which is not nice.

    error: needless `fn main` in doctest
      --> src/wrappers/themis/rust/libthemis-src/src/lib.rs:37:4
       |
    37 | //! fn main() {
       |    ^^^^^^^^^^^^
       |
       = note: `-D clippy::needless-doctest-main` implied by `-D warnings`
       = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_doctest_main

    error: aborting due to previous error

I wouldn't say that explicit main() here is needless and some people
share the same sentiment [1]. However, since this is top-level module
docs, we cannot disable this lint for this place specifically without
turning it off in the entire module. Okay, Clippy, you win.

[1]: rust-lang/rust-clippy#4858

* Fix Cargo with "--features"

No idea what changed here, but for some reason Cargo does not allow to
use "--features" with virtual crates now. However, it has no issues with
it once we're in "themis" crate subdirectory and ask to document all
crates in the workspace anyway.

Apparently, "--features" was not supposed to work with workspaces in the
first place [1], but I have no clue it worked before.

[1]: rust-lang/cargo#5015
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 a pull request may close this issue.

3 participants