-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Bump reviewdog to 0.20.2 #132
base: reviewdog
Are you sure you want to change the base?
Conversation
- Change parameter - Update checkout action versio
- Add if/else to support deprecated fail-on-error option - Simplify deprecated workflow logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anaxite I just had some time to work on #131 I'm glad you took the lead to work on this.
The changes allow both old behavior to coexist (fail_on_error
and fail_level
) so it's great.
I left a minor comment to make the changes working with "any"
as level.
@jdkato Do you think it would be better to remove the compatibility with fail_on_error
or is this approach also to your liking ?
`-level=${vale_code == 1 && (should_fail_on_level === 'error' || | ||
should_fail_on_level === 'warning' || | ||
should_fail_on_level === 'info') ? should_fail_on_level : 'info'}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`-level=${vale_code == 1 && (should_fail_on_level === 'error' || | |
should_fail_on_level === 'warning' || | |
should_fail_on_level === 'info') ? should_fail_on_level : 'info'}` | |
`-level=${vale_code == 1 && (should_fail_on_level === 'error' || | |
should_fail_on_level === 'warning' || | |
should_fail_on_level === 'info') ? should_fail_on_level : 'info'}` |
I'm not sure there is a need to check should_fail_on_level
value. If that's the case, you might need to add 'any'
to be aligned with expected values. I don't think that other cases should be handled ('default'
and 'none'
) since these cases don't make reviewdog exit with 1 (see https://github.com/reviewdog/reviewdog/blob/c53f10c90a146a361c50fe148a78970873ba40ad/fail_level.go#L67)
`-reporter=${core.getInput('reporter')}`, | ||
`-fail-level=${should_fail_on_level}`, | ||
`-filter-mode=${core.getInput('filter_mode')}`, | ||
`-level=${vale_code == 1 && (should_fail_on_level === 'error' || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as the previous one.
@anaxite Could you have a look at my review ? I'm eager to be able to use this :D |
Update
vale-action
to usereviewdog
0.20.2:-fail-level
option as inputfail_level
main.js/ts
flow to only usefail_on_error
whenfail_on_error
is true andfail_level
is set to its default.fail_on_error
input deprecated, per reviewdog documentation.main
GitHub workflow to usefail_level
main
GitHub workflow to useactions/checkout@v4
Resolves #131