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

lib: Remove XXX comment in readline and change to output debuglog #4690

Closed

Conversation

kohei-takata
Copy link
Contributor

refs #4642
Remove a comment that has a word 'XXX'.
And add a line to output debuglog of error.

@@ -380,7 +381,7 @@ Interface.prototype._tabComplete = function() {
self.resume();

if (err) {
// XXX Log it somewhere?
debug(err);
Copy link
Member

Choose a reason for hiding this comment

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

Congratulations your 1st contribution.

You would be better to print more information. e.g.

debug('tab complete error ', err);

like https://github.com/nodejs/node/blob/master/lib/repl.js#L230

@yosuke-furukawa yosuke-furukawa added the good first issue Issues that are suitable for first-time contributors. label Jan 14, 2016
@Fishrock123 Fishrock123 added readline Issues and PRs related to the built-in readline module. and removed good first issue Issues that are suitable for first-time contributors. labels Jan 14, 2016
@Fishrock123
Copy link
Contributor

@kohei-takata Could you please prefix the commit message with readline: instead of lib:?

Also please make sure the commit message (the first line, where the prefix is) is no longer than 50 characters. :)

@jasnell
Copy link
Member

jasnell commented Jan 14, 2016

LGTM with the nits @Fishrock123 and @yosuke-furukawa mentioned addressed.

Remove a comment that has a word 'XXX'.
And add a line to output debuglog of error.
@kohei-takata
Copy link
Contributor Author

@yosuke-furukawa
Thanks 😄
I've changed arguments of debug().

@Fishrock123
Thanks for your review!
I've changed prefix of the commit message and shorten 😄

@jasnell
Thanks!

@cjihrig
Copy link
Contributor

cjihrig commented Jan 15, 2016

LGTM pending CI

@Fishrock123
Copy link
Contributor

@jasnell
Copy link
Member

jasnell commented Jan 15, 2016

Failures in CI look unrelated. Updated commit LGTM

@Trott
Copy link
Member

Trott commented Jan 16, 2016

CI is a little more robust right now then it was 13 hours ago, so let's try CI again:
https://ci.nodejs.org/job/node-test-pull-request/1268/

@Trott
Copy link
Member

Trott commented Jan 16, 2016

CI is green! \o/

@jasnell
Copy link
Member

jasnell commented Jan 16, 2016

Great job @Trott. Great to see.
On Jan 15, 2016 9:24 PM, "Rich Trott" [email protected] wrote:

CI is green! \o/


Reply to this email directly or view it on GitHub
#4690 (comment).

@kohei-takata
Copy link
Contributor Author

@Trott Thanks!

jasnell pushed a commit that referenced this pull request Jan 18, 2016
Remove a comment that has a word 'XXX'.
And add a line to output debuglog of error.

PR-URL: #4690
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Jan 18, 2016

Landed in ad63d35
Thank you @kohei-takata !

@jasnell jasnell closed this Jan 18, 2016
evanlucas pushed a commit that referenced this pull request Jan 18, 2016
Remove a comment that has a word 'XXX'.
And add a line to output debuglog of error.

PR-URL: #4690
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
Remove a comment that has a word 'XXX'.
And add a line to output debuglog of error.

PR-URL: #4690
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
Remove a comment that has a word 'XXX'.
And add a line to output debuglog of error.

PR-URL: #4690
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
Remove a comment that has a word 'XXX'.
And add a line to output debuglog of error.

PR-URL: nodejs#4690
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 11, 2016
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 13, 2016
Remove a comment that has a word 'XXX'.
And add a line to output debuglog of error.

PR-URL: nodejs#4690
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
Remove a comment that has a word 'XXX'.
And add a line to output debuglog of error.

PR-URL: nodejs#4690
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Remove a comment that has a word 'XXX'.
And add a line to output debuglog of error.

PR-URL: nodejs#4690
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
readline Issues and PRs related to the built-in readline module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants