-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: clean up shrinkwrap code #6998
Conversation
969497b
to
cf69e9c
Compare
cf69e9c
to
260ccfa
Compare
let spec = rSpec | ||
if (edge) { | ||
spec = npa.resolve(node.name, edge.spec, edge.from.realpath) | ||
} |
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.
let spec = rSpec | |
if (edge) { | |
spec = npa.resolve(node.name, edge.spec, edge.from.realpath) | |
} | |
const spec = edge ? npa.resolve(node.name, edge.spec, edge.from.realpath) : rSpec |
I 💯% appreciate the de-ternaryifying happenening in this PR. But that does make me want to call out that I think in this case the ternary is easier for me to read. I'm not asking for changes here, just showing my preference for the ternary operator in some cases.
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.
Would an if/else be better here? Is it the overloading of the let
declaration?
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.
It's mostly the order of operations. My inline suggestion is even different than the original ternary because it reverses the order. This is getting in the weeds, but I read the ternary as: if we have an edge, resolve from edge, otherwise its rSpec
where the if statement I read as spec is usually rSpec, but if we have an edge, then resolve from the edge
.
I think even using let spec
and adding an else branch reads better to me. But when writing code like that I usually opt for the ternary because then I don't have to name another intermediate variable. I think naming fatigue is real for me, so it's not that the let declaration is overloaded, it's that I have to Name One More Thing.
I think it should be kept as you have it in the PR since it matches the rest of the file which was in need of this refactor. This was just an excuse to give a little love that I have for ternaries.
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.
Rules are guidelines and should serve their purpose. If we find a case where ternaries are easier to read then let's do it. Use them in your prs and let's find out.
Approved with take-it-or-leave-it comments. |
Gonna land as-is and we can shave the ternary operator yak in a chatroom somewhere. |
Moves to
@npmcli/package-json
for parsingRemoves symbols in favor of private methods/attributes.
Moves "maybe" symbols to getters so they can be accessed during reset
Removes complicated nested ternaries
Inlines some single-use functions