-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Revise for consistency and readability #188
Conversation
896149d
to
31718e7
Compare
Thanks, but I try to avoid patches like this that move stuff around and don't really add much value otherwise, unless it addresses a more serious code architecture problem. Especially, replacing |
I hear ya: "code cleanup unwanted here". I'll avoid submitting similar PR's. You're correct, it doesn't add a feature, fix a bug, or correct a performance issue. It only addresses readability/maintainability. As for saying there's no value in cleaning up, compare the cognitive burden of these two code blocks: // version 1:
notifier.notify({
message: `New ${type} version of ${pkg.name} available! ${
useColor ? color.red(old) : old
} ${useUnicode ? '→' : '->'} ${
useColor ? color.green(latest) : latest
}\n` +
`${
useColor ? color.yellow('Changelog:') : 'Changelog:'
} ${
useColor ? color.cyan(changelog) : changelog
}\n` +
`Run ${
useColor
? color.green(`npm install -g ${pkg.name}`)
: `npm i -g ${pkg.name}`
} to update!`
}) // version 2:
notifier.notify({
message:
`New ${type} version of ${pkg.name} available! ${old} ${arrow} ${latest}\n` +
`${changelogLabel} ${changelog}\n` +
`Run ${installCommand} to update!`
}) Is avoiding switching variables I expanded on what the original author already did. They separated it into 3 blocks:
But, some color logic made its way into section three where static strings are repeated. So, I moved the color logic and strings in section three to section two so all colorizing is done in one spot, no strings are repeated, and that made the template string much more readable/maintainable. It seems worth it to me; that's why I offered it up as a PR. We all write code which needs some cleanup we don't always have time to do ourselves. Happy coding. |
This PR does some code cleanup. It makes some things consistent:
Changes:
useUnicode
in later code by moving it up a scopeI think this makes the code easier to read and maintain.