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

overwrite error status code from config with set_exit_status #589

Merged
merged 7 commits into from
Oct 12, 2021

Conversation

jan-xyz
Copy link
Contributor

@jan-xyz jan-xyz commented Oct 10, 2021

This PR is a result from a discussion in an already closed issue. It copies the functionality from golang/lint to set only the error exit code to 1 and leave everything else untouched.

I didn't add tests for this change, but I can do so, if you point me in a good place to add it.

relates to #241

@jan-xyz jan-xyz changed the title overwrite statuscode from config with set_exit_status overwrite error status code from config with set_exit_status Oct 10, 2021
@jan-xyz jan-xyz marked this pull request as ready for review October 10, 2021 08:35
main.go Outdated
@@ -36,6 +36,10 @@ func main() {
if err != nil {
fail(err.Error())
}
if setExitStatus {
conf.ErrorCode = 1
conf.WarningCode = 2
Copy link
Collaborator

@chavacava chavacava Oct 10, 2021

Choose a reason for hiding this comment

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

I think we should not change the error code for warning otherwise running

revive -set_exit_status

will return 2 in presence of failures (because the default severity is warning)

while

golint -set_exit_status

will return 1

I think we could set the error code for warnings to 1 or set the default severity to error (I prefer the first)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yeah, I set the warning code as well, because otherwise it won't trigger with just that flag. I like your idea to afterwards then also patch the default severity! I'll get right into it.

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 changed it accordingly 😊 please have another look

main.go Outdated Show resolved Hide resolved
@chavacava
Copy link
Collaborator

Hi @jan-xyz, thanks for the PR, I've left a couple of comments

@jan-xyz jan-xyz requested a review from chavacava October 10, 2021 12:02
@chavacava chavacava merged commit 71b31e2 into mgechev:master Oct 12, 2021
@chavacava
Copy link
Collaborator

thanks @jan-xyz !

@jan-xyz
Copy link
Contributor Author

jan-xyz commented Oct 12, 2021

No worries! I noticed you went in the end with seeing both codes to 1, did I misunderstand you on the weekend, or did you change your mind? Would love to better understand what I could've done better, if you don't mind sharing 😊

@chavacava
Copy link
Collaborator

chavacava commented Oct 15, 2021

Hi @jan-xyz, sorry for the late response.

did I misunderstand you on the weekend, or did you change your mind?

In my (original) comment I suggested to set the default severity to warning, and just few seconds after commenting I've modified the comment to add the alternative of setting to 1 the error code for warnings. I think you read the first version of my comment. Sorry for that, my bad.
(You can access both versions of my comment from the "edited" drop-down)

Would love to better understand what I could've done better, if you don't mind sharing
You did it very good and I've learnt to avoid modifying comments :)

Thanks again

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

Successfully merging this pull request may close these issues.

2 participants