SonarLint pass against detected bugs and code smell issues #3135
Conversation
|
|
@msimerson I'll go back over it after I get some errands done. I'll also update the description since "fix" is arbitrary: it does find bugs and "code smell violations", but I guess if they don't hurt, there's not an immediate issue? |
unquietwiki
changed the title
Not on GitHub. Fix has special meaning, and it links PRs and issues together and has special actions related to those linked issues and PRs. |
|
Didn't think of it that way, till you said so just now. In past workflows, I'm used to there being JIRA workflows that could track projects together; I'm not normally an Agile/Waterfall guy, so my use of those would be in project meta groups, tasks, and subtasks. |
|
@msimerson Hey, I was working on the SonarLint TODOs, and kept running into testing failures. Turns out the DNS record for the anti-spam got deleted; there was a planned cutover a year+ ago to a new one. |
|
@msimerson Okay... I completed most of the TODOs I generated, and the testing is working. I started looking at the testing code, but gave up after running into some issues. The testing code's gonna need its own separate review aside from that DNSBL issue I had to resolve; otherwise, I think things are fine aside from any P42 stuff you still want to do, as well as anything Lars turns up and/or explore use of Prettier to deal with any ternary messes. |
| @@ -205,7 +207,7 @@ class HMailItem extends events.EventEmitter { | |||
| // assume string | |||
| const matches = /^(.*?)(:(\d+))?$/.exec(mx); | |||
| if (!matches) { | |||
| throw ("get_mx returned something that doesn't match hostname or hostname:port"); | |||
| throw (new Error("get_mx returned something that doesn't match hostname or hostname:port")); | |||
There was a problem hiding this comment.
I agree that anything throwing an error should throw new Error('....'). But...we haven't always done that. Have you verified that any code that will catch this error is expecting an error object and not just a string?
There was a problem hiding this comment.
It passes the test suite, and I made this change aside from the other issues we were having with P42 if-else changes that I have Lars looking at.
There was a problem hiding this comment.
Our test suite isn't complete, and is not alone good enough to validate a change like this. Either do the legwork or revert this change.
There was a problem hiding this comment.
- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error
- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/throw
If anything, we're not even utilizing the full power of Error here; there are newer ES20xx commands that can do error handling like you would in C#, Nim, or other languages. And I don't see much "catch" of throws, either. This still seems legit to me.
There was a problem hiding this comment.
You're preaching at the choir. The question was never whether or not this was legit or a good idea. The question is whether or not this BREAKS OUR CODE because the caller(s) of this function are expecting a different type of throw. If they are, more changes are required.
There was a problem hiding this comment.
@msimerson sorry I didn't see this sooner; I wasn't getting fully updated with notifications. I have everything else fixed so far; I'll dig into this in a bit. Thank you for your extreme patience.
|
For now, I've given this PR all the time I'm willing to. I am generally a fan of linters and automated code improvement tools, and reducing "detected bugs and code smells" sure sounds good, but SonarLint doesn't live up to that standard. Too many of the changes here aren't improvements and worse, several of the changes introduce bugs. This PR needs a fair bit more work and extremely careful review before it can be merged. |
|
@msimerson That's fair, and it looks like you cherry-picked the stuff you wanted? I'm still worried about that potential CVE you & I were haggling over; otherwise, I'm fine with where things are at. If there's any other tooling I'd suggest at this point, that might be better than SonarLint, would be Synk. I use it on my repos. |
Easy, create a PR that does just that. |
|
@msimerson I'll take a stab on that again & try to make something we can agree on. Again, thank you for your patience on this: I plan to use this as part of a server project I'm putting together. |
|
@msimerson I submitted #3139 with one more take on the connection.js issue. I have an explicit 5-part test on triggering the disconnect. |
Fixes #3120
SonarLint fixes outside of "tests" (didn't wanna mess with those for now).