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

Annotated github ci output #778

Closed
jbergstroem opened this issue Jul 12, 2023 · 5 comments
Closed

Annotated github ci output #778

jbergstroem opened this issue Jul 12, 2023 · 5 comments

Comments

@jbergstroem
Copy link

jbergstroem commented Jul 12, 2023

It would be great to get typos annotated as part of a PR check. The most straightforward path is probably through a problem matcher - it could look something like this:

{
  "problemMatcher": [
    {
      "owner": "typos",
      "applyTo": "allDocuments",
      "pattern": [
        {
          "regexp": "^(error|warning): (.+)\\n  --> (.+?):(\\d+):?(\\d+)?$",
          "file": 3,
          "line": 4,
          "column": 5,
          "severity": 1,
          "message": 2
        }
      ]
    }
  ]
}

Happy to open a PR if it is desired!

@epage
Copy link
Collaborator

epage commented Jul 14, 2023

We had annotations in the past. I'm assuming they aren't working. We were using json output for it

https://github.com/crate-ci/typos/blob/master/action/entrypoint.sh#L64

I think I'd prefer using json over a problemMatch

Alternatively, there is also #594

@jbergstroem
Copy link
Author

We had annotations in the past. I'm assuming they aren't working. We were using json output for it

Oh! Yeah, I haven't seen it. I can take a look at fixing that instead then.

@jbergstroem
Copy link
Author

jbergstroem commented Jul 15, 2023

We had annotations in the past. I'm assuming they aren't working. We were using json output for it

Oh! Yeah, I haven't seen it. I can take a look at fixing that instead then.

Just checked - the output looks just fine to me. I think the "problem" is that the warnings that typos generate for files in my repository weren't part of the pull request. I guess that's one benefit of a problem matcher ("applyTo": "allDocuments")

@epage
Copy link
Collaborator

epage commented Jul 20, 2023

For most projects I've been on, we've preferred to not touch unrelated code in the review but to save those for another PR. This means annotating code unrelated to the PR is unactionable. By providing unactional annotations, we are creating extra noise for people that would make them more likely to ignore all annotations or turn off the tool.

Unless there is additional information, then I'm going to assume the annotations are working as expected, that we don't want to expand the scope of the annotations, and for any further change in this area, we'd more likely want #594, I'm leaning towards closing this.

@jbergstroem
Copy link
Author

Appreciate the consideration! Thanks for your input.

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

No branches or pull requests

2 participants