-
Notifications
You must be signed in to change notification settings - Fork 145
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
feat: Reselect existing contributions when updating a user #60
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.
This looks great! Thank you! @machour, could you give this a review as well?
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.
Hi @chrisinajar and thank you so much for contributing 🎉
I left you some remarks that should be taken care of.
Additionally, I'm not sure about this PR in its current state, here's why:
I recently introduced an additional check in #56 to make sure that the user select at least one entry in the choices.
From my experience, users often mistakenly highlight the desired entry in the list, then hit "Enter" instead of "Space", believing that this will select the highlighted answer.
The check I added helps with that as it forbid the user from selecting nothing by mistake.
With your proposed change, there is always something selected, so if the user make the same mistake again, it will go silently un-noticed.
Do you think you can update the PR to check that something changed (i.e. old contributions
values is different from the new one), and print an error if this is not the case? Something like: "You didn't update contributions, make sure you use the space bar to select/unselect entries"
/cc @kentcdodds for inputs if any
cli.js
Outdated
@@ -56,7 +56,7 @@ function addContribution(argv) { | |||
var username = argv._[1]; | |||
var contributions = argv._[2]; | |||
// Add or update contributor in the config file | |||
return updateContributors(argv, username, contributions) | |||
return updateContributors(argv, username, contributions, argv.contributors) |
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.
We're already passing argv
as the first parameter.
Is this really needed?
lib/contributors/prompt.js
Outdated
@@ -18,7 +18,7 @@ var contributionChoices = _.flow( | |||
}) | |||
); | |||
|
|||
function getQuestions(options, username, contributions) { | |||
function getQuestions(options, username, contributions, allContributions) { |
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.
should be named allContributors
instead (but it can be accessed from options.contributors
)
lib/contributors/prompt.js
Outdated
// default values for contributions when updating existing users | ||
return allContributions | ||
.filter((entry) => entry.login.toLowerCase() === answers.username.toLowerCase()) | ||
.reduce((memo, entry) => memo.concat(entry.contributions), []); |
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.
What does memo
stands for?
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 a fairly common argument name for the first argument in a reducer. Sometimes also acc
for accumulator or even state
. I personally don't really care. In this case, perhaps allEntries
would be preferable.
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.
Thank you for the explanation! Good for me as well then :)
I'm good to defer to @machour on this 👍 |
Fixed the usability case, removed the dumb extra param (didn't realize options and argv were the same thing while working backwards). I also found an error when it's launched by cli |
@chrisinajar thank you for the update. I re-tested the PR and came across an unhandled case: removing a contribution type doesn't work. I think this never worked as previously contributions weren't re-selected and were kept anyways, but it just more clear with this PR. Do you want to fix it here, or do we keep log that in a new issue and merge this baby in as is? |
That's actually an interesting usability issue. There's currently no way to remove accidental contribution types added to the list. If we changed this to set (instead of merge) the contributions, then direct cli usage ( I guess this change just brings to light an awkward usability interaction. |
Version 4.6.0 is out with your contribution 🎉 |
* docs: add implementations * Rename IMPLEMENTATIONS to IMPLEMENTATIONS.md * Update IMPLEMENTATIONS.md
When you choose to update an existing contributor it now populates the initial selection with their current values, if any.