Skip to content

Conversation

ntBre
Copy link
Contributor

@ntBre ntBre commented Sep 9, 2025

Summary

This is the GitHub analog to #20117. This PR prepares to add a GitHub output format to ty by moving the implementation from ruff_linter to ruff_db. Hopefully this one is a bit easier to review commit-by-commit. Almost all of the refactoring this time is in the first commit, then the second commit adds the new OutputFormat variant and moves the file into ruff_db. The third commit is just a small touch up to use a private method that accommodates ty files so that we can run the tests and update/move the snapshots.

I had to push a fourth commit to fix and test diagnostics without a span/file.

Test Plan

Existing tests

@ntBre ntBre added internal An internal refactor or improvement diagnostics Related to reporting of diagnostics. labels Sep 9, 2025
Copy link
Contributor

github-actions bot commented Sep 9, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Comment on lines +47 to +51
",line={row},col={column},endLine={end_row},endColumn={end_column}::",
row = start_location.line,
column = start_location.column,
end_row = end_location.line,
end_column = end_location.column,
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 think we could omit these entirely if there's no range, based on the docs, but I stuck with unwrap_or_default for now since that's what we were already doing for notebooks.

this prepares to move the code to `ruff_db` by using a `&dyn FileResolver`
instead of the `EmitterContext` directly and by handling the cases where a span
or range is missing. I think this also fixes a latent bug where we were only
falling back on a default `LineColumn` for notebooks for the start location.
`end_location` was used directly without respecting the `is_notebook` check.

note that the one remaining `unwrap` call will disappear after moving the code
to `ruff_db`. the `UnifiedFile` method we need is still private
this is the one piece of the refactor we couldn't do in the first commit, just
because `diagnostic_source` is private. it probably doesn't have to be private,
but it also hasn't needed to be public once we finish moving each output format
over
See the example error message in the Github docs:
<https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands#setting-an-error-message>

```
::error file={name},line={line},endLine={endLine},title={title}::{message}
```

even if we don't have `file` or `line`, we still need the `::` before `message`.
@ntBre ntBre marked this pull request as ready for review September 10, 2025 19:55
@ntBre ntBre requested review from BurntSushi and removed request for carljm, dcreager and sharkdp September 10, 2025 19:55
Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

Nice! The commit breakdown was perfect, thank you!

I like the unwrap/expect removals. :-)

@ntBre ntBre merged commit 5bf6977 into main Sep 11, 2025
35 checks passed
@ntBre ntBre deleted the brent/github branch September 11, 2025 17:11
ntBre added a commit that referenced this pull request Sep 17, 2025
## Summary

This PR wires up the GitHub output format moved to `ruff_db` in #20320
to the ty CLI.

It's a bit smaller than the GitLab version (#20155) because some of the
helpers were already in place, but I did factor out a few
`DisplayDiagnosticConfig` constructor calls in Ruff. I also exposed the
`GithubRenderer` and a wrapper `DisplayGithubDiagnostics` type because
we needed a way to configure the program name displayed in the GitHub
diagnostics. This was previously hard-coded to `Ruff`:

<img width="675" height="247" alt="image"
src="https://github.com/user-attachments/assets/592da860-d2f5-4abd-bc5a-66071d742509"
/>

Another option would be to drop the program name in the output format,
but I think it can be helpful in workflows with multiple programs
emitting annotations (such as Ruff and ty!)

## Test Plan

New CLI test, and a manual test with `--config 'terminal.output-format =
"github"'`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

diagnostics Related to reporting of diagnostics. internal An internal refactor or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants