[rustdoc] Correctly handle output options with --show-coverage

[GuillaumeGomez]

View all comments

Fixes #158929.

The -o option was ignored with --show-coverage. This PR takes it into account and makes it closer to how these options are handled in the rest of rustdoc.

Considering calculate_doc_coverage was never really a pass and that I needed RenderOptions (for the -o option), I moved it out of passes.

r? @camelid

[rustbot]

camelid is currently at their maximum review capacity.
They may take a while to respond.

[GuillaumeGomez]

Ah, maybe @Urgau then?

r? @Urgau

[Urgau]

I haven't looked at the implementation, but it seems a bit weird to me to have an flag called --show-coverage that doesn't show anything when used with -o. Are we worried about confusing users?

#158929 suggests updating the documentation instead.
cc @aDotInTheVoid (in case you have an opinion)

[GuillaumeGomez]

I haven't looked at the implementation, but it seems a bit weird to me to have an flag called --show-coverage that doesn't show anything when used with -o. Are we worried about confusing users?

#158929 suggests updating the documentation instead. cc @aDotInTheVoid (in case you have an opinion)

If you want to generate the output in a file, that seems kinda logical to not have output on stdout.

[Urgau]

If you want to generate the output in a file, that seems kinda logical to not have output on stdout.

100% agree, but the CLI option is called --show-..., that implies (at least to me) -o - (i.e. the current behavior). Maybe we should have --coverage?

Is there a way to know if some users are currently relying on the current behavior? (since this is a breaking change)

[GuillaumeGomez]

docs.rs. We could rename the option, I don't mind. But something like --doc-coverage seems better than just coverage, no?

[Urgau]

Yeah, --doc-coverage seems fine. I'm mostly worried about the breaking change to --show-coverage, so if -o is respected only for this new option, that seems fine to me.

[GuillaumeGomez]

It's a nightly only option, so breaking changes are fine. Although in this case, as long as you don't use -o, there is no change.

[Urgau]

Oh! I thought --show-coverage was stable, never-mind about the breaking change then.

Though I still think renaming it to --doc-coverage would be better.

[GuillaumeGomez]

A follow-up and an issue about doing it would be good since we'll need an FCP. ;)

[Urgau]

Created #159691 for the rename.

[Urgau]

@bors r+

[rust-bors]

📌 Commit 1010137 has been approved by Urgau

It is now in the queue for this repository.

[rust-bors]

This pull request was unapproved.

This PR was contained in a rollup (#159905), which was unapproved.

View changes since this unapproval

[camelid]

 failures:
  
  ---- [run-make] tests\run-make\rustdoc-show-coverage stdout ----
  
  error: rmake recipe failed to complete
  status: exit code: 101
  command: "C:\\a\\rust\\rust\\build\\aarch64-pc-windows-msvc\\test\\run-make\\rustdoc-show-coverage\\rmake.exe"
  stdout: none
  --- stderr -------------------------------
  "C:\\a\\rust\\rust\\build\\aarch64-pc-windows-msvc\\stage2\\bin\\rustdoc.exe" "foo.rs" "-Zunstable-options" "--show-coverage" "--target=aarch64-pc-windows-msvc"
  output status: `exit code: 0`
  === STDOUT ===
  +-------------------------------------+------------+------------+------------+------------+
  | File                                | Documented | Percentage |   Examples | Percentage |
  +-------------------------------------+------------+------------+------------+------------+
  | foo.rs                              |          0 |       0.0% |          0 |       0.0% |
  +-------------------------------------+------------+------------+------------+------------+
  | Total                               |          0 |       0.0% |          0 |       0.0% |
  +-------------------------------------+------------+------------+------------+------------+
  
  
  
  === STDERR ===
  
  
  
  "C:\\a\\rust\\rust\\build\\aarch64-pc-windows-msvc\\stage2\\bin\\rustdoc.exe" "foo.rs" "-Zunstable-options" "--show-coverage" "-o" "-" "--target=aarch64-pc-windows-msvc"
  output status: `exit code: 0`
  === STDOUT ===
  +-------------------------------------+------------+------------+------------+------------+
  | File                                | Documented | Percentage |   Examples | Percentage |
  +-------------------------------------+------------+------------+------------+------------+
  | foo.rs                              |          0 |       0.0% |          0 |       0.0% |
  +-------------------------------------+------------+------------+------------+------------+
  | Total                               |          0 |       0.0% |          0 |       0.0% |
  +-------------------------------------+------------+------------+------------+------------+
  
  
  
  === STDERR ===
  
  
  
  "C:\\a\\rust\\rust\\build\\aarch64-pc-windows-msvc\\stage2\\bin\\rustdoc.exe" "foo.rs" "-Zunstable-options" "--show-coverage" "--output-format=json" "--target=aarch64-pc-windows-msvc"
  output status: `exit code: 0`
  === STDOUT ===
  {"foo.rs":{"total":3,"with_docs":0,"total_examples":2,"with_examples":0}}
  
  
  
  === STDERR ===
  
  
  
  "C:\\a\\rust\\rust\\build\\aarch64-pc-windows-msvc\\stage2\\bin\\rustdoc.exe" "foo.rs" "-Zunstable-options" "--show-coverage" "--output-format=json" "-o" "-" "--target=aarch64-pc-windows-msvc"
  output status: `exit code: 0`
  === STDOUT ===
  {"foo.rs":{"total":3,"with_docs":0,"total_examples":2,"with_examples":0}}
  
  
  
  === STDERR ===
  
  
  
  "C:\\a\\rust\\rust\\build\\aarch64-pc-windows-msvc\\stage2\\bin\\rustdoc.exe" "foo.rs" "-Zunstable-options" "--show-coverage" "-o" "doc" "--target=aarch64-pc-windows-msvc"
  output status: `exit code: 0`
  === STDOUT ===
  Generated output into "doc\\foo.txt"
  
  
  
  === STDERR ===
  
  
  
  assert_contains_regex:
  === HAYSTACK ===
  Generated output into "doc\\foo.txt"
  
  === NEEDLE ===
  Generated output into "doc[/\\]foo.txt"
  
  
  thread 'main' (9516) panicked at C:\a\rust\rust\tests\run-make\rustdoc-show-coverage\rmake.rs:40:5:
  regex was not found in haystack
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
  ------------------------------------------
  
  ---- [run-make] tests\run-make\rustdoc-show-coverage stdout end ----
  
  failures:
      [run-make] tests\run-make\rustdoc-show-coverage

[GuillaumeGomez]

@bors try jobs=aarch64-msvc-1

[rust-bors]

💔 Test for 49bbeb7 failed: CI. Failed job:

• try - aarch64-msvc-1 (web logs, enhanced plaintext logs)

[GuillaumeGomez]

And fixed CI...

[GuillaumeGomez]

@rustbot ready

[Urgau]

@bors r=Urgau,aDotInTheVoid rollup

[rust-bors]

📌 Commit 41704bd has been approved by Urgau,aDotInTheVoid

It is now in the queue for this repository.