codespell-project / actions-codespell

MIT License
74 stars 19 forks source link

only_warn should produce warnings not errors #78

Open doc-sheet opened 3 weeks ago

doc-sheet commented 3 weeks ago

Now all messages show as ::error regardless of only_warn value.

DimitriPapadopoulos commented 3 weeks ago

What do you mean by "Now"? Did anything change recently? Since when?

How do you set only_warn exactly?

doc-sheet commented 3 weeks ago

now == last time i've tested it - few days ago at latest version I believe

It works well as action

      - name: check spelling
        uses: codespell-project/actions-codespell@406322ec52dd7b488e48c1c4b82e2a8b3a1bf630
        with:
          only_warn: true
          check_filenames: true
          path: >-
                some paths here

The only minor issue - one i'm talking about - is only_warn makes codespell to not fail action run by using return code 0. But messages it pins on PR diff still show as errors.

DimitriPapadopoulos commented 3 weeks ago

I see. So it's not a new or recent issue. The word "New" simply means you're running the latest version.

DimitriPapadopoulos commented 3 weeks ago

I have no clue how to fix this myself. If you know, please do not hesitate to provide a pull request.

DimitriPapadopoulos commented 3 weeks ago

But then, it looks like only_warn does what it is supposed to do:

Only warn about problems. All errors and warnings are annotated in Pull Requests, but it will act like everything was fine anyway. (In other words, the exit code is always 0.)

doc-sheet commented 3 weeks ago

Yeah i just suggest to replace all error annotations as warnings. Sadly I don't fully understand how this action works so I can't suggest proper fix.

Looks like https://github.com/codespell-project/actions-codespell/blob/406322ec52dd7b488e48c1c4b82e2a8b3a1bf630/codespell-problem-matcher/codespell-matcher.json#L20 should be WARNING always if only_warn is enabled.

DimitriPapadopoulos commented 2 weeks ago

But that's not the documented behaviour as far as I an tell. That would be a breaking change involving a major version bump, wouldn't it?

On the other hand, that's what other actions do. See for example https://github.com/actions/dependency-review-action:

warn-only

When set to true, the action will log all vulnerabilities as warnings regardless of the severity, and the action will complete with a success status. This overrides the fail-on-severity option.

Finally, what exact difference does that make in practice? How do warnings and errors differ in the output of CI jobs?

doc-sheet commented 2 weeks ago

what exact difference does that make in practice?

It is just a small recolor of things :) warning error

doc-sheet commented 2 weeks ago

not the documented behaviour as far as I an tell.

TBH I see that matcher json for first time and I didn't even see docs of how to use it.

Usually i just write something like echo ::error::blabla or echo ::warning::blabla where it is easy to control

peternewman commented 2 weeks ago

So I based this action on this one and just took the names from there: https://github.com/TrueBrain/actions-flake8?tab=readme-ov-file#parameter-only_warn

FWIW the matcher stuff is documented here: https://github.com/actions/toolkit/blob/main/docs/problem-matchers.md

The main benefit is you don't need to make any changes to the tool generally, you just write some Regex to match its output (which also makes things like https://github.com/codespell-project/sort-problem-matcher possible where they probably wouldn't accept a PR to modify sort for GitHub usage).

Unfortunately if the docs are correct you can't just overwrite the number in that config as they Regex group matches, you'd have to either modify Codespell itself or pre-process its output.

Personally I'd say there's an argument for both use cases, you might want to flag the correct severity of found issues, whilst not failing the whole build (as it currently does) but maybe in retrospect that should have been a differently named option. Then there's what you're asking for where all logged alerts are at the warning level. But do you also want the job to be forced success too?

peternewman commented 2 weeks ago

Correction, apparently their docs are rubbish and you can sort of override severity, but you'd need a different matcher: https://github.com/orgs/community/discussions/5924#discussioncomment-5722148