Skip to content

fix: silent warning message & wrong exit status check #81

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

Merged
merged 5 commits into from
Aug 25, 2023
Merged

fix: silent warning message & wrong exit status check #81

merged 5 commits into from
Aug 25, 2023

Conversation

xiaoshihou514
Copy link
Collaborator

Just been testing with selene and found two issues.

  • on_failed was silent again(process %s exited ... message did not show up), so I reverted a change.
  • Finally I found that the exist status check was not behaving as expected as it exits with exit code 1 and an empty stderr(which should have been fine!) Guess I wasn't paying enough attention previously 😅

@xiaoshihou514
Copy link
Collaborator Author

Maybe related to #77 , idk

if exit_code == 0 and num_stderr_chunks == 0 then
coroutine.resume(co, table.concat(chunks))
else
if exit_code ~= 0 and num_stderr_chunks ~= 0 then
Copy link
Member

Choose a reason for hiding this comment

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

there should not be or ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That makes it equivalent to the original code, where either a non-zero exit code or non empty stderr indicates a lint failure. The problem is linters return non-zero exit code (for instance selene returns 1 when it found errors in given file) when the lint succeeds. So here both need to be fulfilled before one could determine that the lint fails.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change not the same as keeping the original code and replacing the and with an or?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this change not the same as keeping the original code and replacing the and with an or?

It is, but I feel this way it's clearer what is being achieved

@barrett-ruth
Copy link
Contributor

Thanks for this, was suspect on those changes I implemented - my bad.

@xiaoshihou514
Copy link
Collaborator Author

Thanks for this, was suspect on those changes I implemented - my bad.

I should have done more testing! My laptop wasn't available at that timing :( It's definitely best to be more cautious next time, so as not to break someone's workflow.

@xiaoshihou514 xiaoshihou514 merged commit d985721 into nvimdev:main Aug 25, 2023
xiaoshihou514 added a commit that referenced this pull request Aug 25, 2023
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.

3 participants