Skip to content

Always print coverage findings in pyrefly coverage check --fail-under=#4156

Open
jorenham wants to merge 2 commits into
facebook:mainfrom
jorenham:coverage/always-emit-warnings
Open

Always print coverage findings in pyrefly coverage check --fail-under=#4156
jorenham wants to merge 2 commits into
facebook:mainfrom
jorenham:coverage/always-emit-warnings

Conversation

@jorenham

Copy link
Copy Markdown
Collaborator

Summary

Right now, pyrefly coverage report --fail-under=<something below 100> does not print the findings if the coverage is above the threshold. I think it would be more useful if those findings would be printed regardless of whether it passes (i.e. exits 0). And if the user doesn't want to see those findings for some reason, then they could always use the existing --output-format=omit-errors flag to disable this.

Test Plan

Manually verified that the missing coverage warnings are now always emitted.

@meta-cla meta-cla Bot added the cla signed label Jul 15, 2026
@jorenham jorenham changed the title Coverage/always emit warnings Emit config warnings in pyrefly coverage Jul 15, 2026
@jorenham jorenham changed the title Emit config warnings in pyrefly coverage Always print coverage findings in pyrefly coverage check --fail-under= Jul 15, 2026
@github-actions

Copy link
Copy Markdown

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@NathanTempest NathanTempest left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As always, thank you for your efforts in improving Pyrefly coverage @jorenham. The implementation is clean (nice to drop the Option in favor of a clap default_value_t, and moving write_errors_to_console above the branch keeps findings→summary ordering consistent), and the docs update is thorough. The reasoning is reasonable too: findings go to stdout, the summary to stderr, and --output-format=omit-errors is the escape hatch, so exit-code-only CI is unaffected.

The one thing I want to confirm before merging is the default-behavior change itself. Printing findings even when coverage passes changes the default output for everyone, and on large codebases that could be a lot of output on every green run. Looping in @maggiemoss for a quick sign-off on making that the default vs. keeping quiet-on-success. Assuming she's good with it, this is ready to go.

@NathanTempest NathanTempest self-assigned this Jul 16, 2026
@NathanTempest
NathanTempest requested a review from maggiemoss July 16, 2026 16:31
@jorenham

jorenham commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator Author

The one thing I want to confirm before merging is the default-behavior change itself. Printing findings even when coverage passes changes the default output for everyone, and on large codebases that could be a lot of output on every green run.

Yea good point. I don't expect that this will be a problem for CI though, since it still exits 0 like it used to in those cases.
Also, pyrefly coverage is very new, so I doubt that many large projects will be using it at this point. The only large codebases that I'm aware of use this are numpy and scipy-stubs, and both have 100% type-coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants