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

fix: always let printed reports send to stdout #512

Closed
wants to merge 4 commits into from

Conversation

KalleChen
Copy link

@KalleChen KalleChen commented Oct 20, 2023

Why do we need this PR?

  • If there're Errors in solhint reports, the output will sent to std_err because it exit(1). I think this should be always sent to std_out since there're actually no "errors" in the process, just "Error" against the solhint rules.

This PR is in order to fix this

Also, I have a previous PR for this, since this repo has not been maintained then.

@KalleChen KalleChen changed the base branch from master to develop October 20, 2023 16:31
@dbale-altoros
Copy link
Collaborator

@KalleChen we really appreciate getting involved
Can you give me further info of what your intentions are with this pr
Take into account this is making several e2e tests fail... so maybe we can work something out with this if I can understand better your point

@dbale-altoros dbale-altoros self-requested a review October 26, 2023 13:10
@dbale-altoros dbale-altoros added the awaiting user feedback awaiting user feedback label Oct 26, 2023
@KalleChen
Copy link
Author

KalleChen commented Oct 26, 2023

@KalleChen we really appreciate getting involved Can you give me further info of what your intentions are with this pr Take into account this is making several e2e tests fail... so maybe we can work something out with this if I can understand better your point

@dbale-altoros Thanks for your reply!

The problem is that we use null-ls to print the solhint on nvim. And this issue indicates that the reports output from solhint sometimes sent to stderr. You can just start from my comment

And I found that this function will check if the reposts include error then exit the process with 1

function exitWithCode(reports) {
  const errorsCount = reports.reduce((acc, i) => acc + i.errorCount, 0)
  process.exit(errorsCount > 0 ? 1 : 0)
}

I also found using process.exit(1) will let the output of console.log sent to stderr not stdout, and this cause the issue above.
The problem is if there's only warning in solhint report, it sent it to stdout, but if there's one error in report, it will sent it to stderr.

And I think this should not happen, because there's actually no "error" happen in the linting process, just 'error flag' in report.

Therefore, this PR is trying to remove the exitWithCode function.

If there's something confusing or I have got something wrong, pls let me know. thanks again!

btw, I will get some time to investigate why e2e test failed.

@dbale-altoros
Copy link
Collaborator

I think I understand your point... let me see what we can do...
We are planning a new version pretty soon... stay tuned...

Join our discord:
https://discord.gg/4TYGq3zpjs

@KalleChen
Copy link
Author

KalleChen commented Oct 26, 2023

@dbale-altoros
Okay, I understand. I don't think this is a very urgent matter, so there's no need to rush.

I will be looking forward to seeing the new version of solhint. Thanks for your help and maintain this project.

@dbale-altoros
Copy link
Collaborator

@KalleChen thanks to you for your contribution
I will try to squeeze this into next release

@KalleChen
Copy link
Author

@dbale-altoros thanks! if there's anything I could help you could just let me know

@dbale-altoros
Copy link
Collaborator

#554

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting user feedback awaiting user feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants