Skip to content
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

multiple reviewers should be space separation. #35

Closed
jetersen opened this issue Jul 21, 2022 · 9 comments · Fixed by #41
Closed

multiple reviewers should be space separation. #35

jetersen opened this issue Jul 21, 2022 · 9 comments · Fixed by #41
Labels
enhancement New feature or request

Comments

@jetersen
Copy link
Contributor

const reviewersToBeParsed = reviewerMatches[1].split(",");

@timja this should potentially handle spaces instead of comma separation.

One they cannot have spaces in their username.
Two by default the auto complete fills in a space when hitting enter for completion.

@timja @jetersen @etc

@lemeurherve
Copy link
Contributor

I'd still keep the comma to be coherent with the /label command with a comma separator accepting labels with spaces

@jetersen
Copy link
Contributor Author

jetersen commented Jul 21, 2022

Should be relatively easy

Believe you can do

const reviewersToBeParsed = reviewerMatches[1].split("[, ]");

@jetersen
Copy link
Contributor Author

I'd suggest writing some tests 😉

updatecli/updatecli-action#100

@timja
Copy link
Owner

timja commented Jul 21, 2022

It's next on my plan 😄
I've done some refactoring recently to make them easier to implement

#7


and yes makes sense to allow spaces here and I agree with @lemeurherve that comma should be kept for consistency with labels which can't have a space separator

@timja timja added the enhancement New feature or request label Jul 21, 2022
@timja
Copy link
Owner

timja commented Jul 25, 2022

Should be relatively easy

Believe you can do

const reviewersToBeParsed = reviewerMatches[1].split("[, ]");

The tests failed with this, I just did:

  const separator = reviewers.includes(",") ? "," : " ";
  const split = reviewers.split(separator);

@jetersen
Copy link
Contributor Author

jetersen commented Jul 25, 2022

@timja

The correct syntax is /regex/ by bad :D

const split = reviewers.split(/[,\s]/);

@timja
Copy link
Owner

timja commented Jul 25, 2022

sure not sure if I really want to support both at the same time anyway, so my approach should be fine?

@jetersen
Copy link
Contributor Author

Why not regex can handle splitting correctly for both? 😅

@timja
Copy link
Owner

timja commented Jul 25, 2022

yeah sure have merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants