-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Removing "No Operand in Assignment" Usage #120
Conversation
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.
Overall looking good, I think this one change is meant to be assignment not comparison, though.
Thanks for doing this, IMO these complicated lines really do make more sense separated into more lines.
Co-authored-by: DeeDeeG <[email protected]>
src/upgrade.js
Outdated
if (pack = this.getIntalledPackage(name)) { | ||
let pack = this.getInstalledPackage(name); |
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.
You'll probably be surprised why this was failing the tests: the actual function name in this file has been getIntalledPackage
(note the typo, a missing "s" in "intalled") this whole time, since 2014 (atom/apm@c8d61000#diff-0aa64babcef87ba238fed061035f2fe14c587092a7339b55c5e4033c37370d70R46).
(Regarding the singular getIntalledPackage
, not to be confused with the plural getInstalledPackages
("Packages") which has/had no typo.)
So, we can either revert the spelling here to the previous (typoed but defined) function name here, or rename the function elsewhere in the file that this line calls, so as to not have a typo in its name, lol.
But yeah, this broke tests by fixing a typo, I honestly did not see that coming.
Thanks again for working on this! Nice to see readability improvements to the code!!!!!! We sure need it, as debugging this PR has helped prove, IMO!!! Thank you!
Co-authored-by: DeeDeeG <[email protected]>
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.
Yeeeeeeeeeaaaaaaaaaah 😎
let's do it
(More readable code that at least mostly makes sense is both a good thing and has historically been all too rare in ppm repo. We've come a long way!!!!!!!)
With PPM now properly setup on Codacy, the next two biggest suggestions are the following:
is not
operators that weren't decaffed cleanly, and also a single instance of what appears to be a typo of=
rather than==
With this issues addressed, we don't expect to see any change in hope behavior ourselves.