Skip to content

Conversation

@jchue
Copy link
Contributor

@jchue jchue commented Dec 8, 2019

Split out and revised changes originally in #1612

@lex111 lex111 requested a review from paroche December 19, 2019 20:16
Copy link
Collaborator

@paroche paroche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All changes OK. I made a couple suggestions for pretty minor improvements.

@iliakan
Copy link
Member

iliakan commented Dec 27, 2019

@paroche should we merge it in its current state?

@paroche
Copy link
Collaborator

paroche commented Dec 27, 2019

Would be OK to merge as is. I had some small suggestions but they could be done later, or not at all.

When you "assign" it to me, what exactly does that mean?

@iliakan
Copy link
Member

iliakan commented Dec 27, 2019

@paroche that means I'm unable to resolve the matter, as it's not about JavaScript, but rather about the English language, the wording and the syntax.

So I kindly ask you to take a look, request changes and merge if you deem appropriate :)

@paroche
Copy link
Collaborator

paroche commented Dec 27, 2019

OK. Just seems like you've asked me to look at things before without "assigning" them to me. I guess it's the "merge if I deem appropriate" part that might be different.

Though with these more extensive changes where I agree with some, not with others, and for others might have a third (and/or forth) suggestion, I'm not sure how to proceed -- don't think I can push it piece-meal or make revisions (except after pushing it all). And may want to give the author of the PR a chance to make revisions or comments first. So then I just comment.

@iliakan
Copy link
Member

iliakan commented Dec 28, 2019

@paroche you use create a review and "request changes" using the button in the right-upper corner, right?

Then the PR author should review and update their PR.

@jchue
Copy link
Contributor Author

jchue commented Jan 2, 2020

Updated

@paroche paroche merged commit a09d162 into javascript-tutorial:master Jan 2, 2020
@paroche
Copy link
Collaborator

paroche commented Jan 2, 2020

OK. Too bad approving it piece-meal isn't an option (or doesn't seem to be). Seems like would be easier for long series of edits in one PR.

@jchue
Copy link
Contributor Author

jchue commented Jan 2, 2020

Agreed.

@paroche
Copy link
Collaborator

paroche commented Jan 2, 2020

That was in response to Ilya's last comment, but I'm glad you agree. Is harder to keep track this way and it can lead to a lot of redundancy in reviewing changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants