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

lin: moved setting of this._decoder up in readline.Interface #30991

Closed

Conversation

ww9rivers
Copy link

Checklist

@nodejs-github-bot nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Dec 16, 2019
Trott
Trott previously requested changes Dec 17, 2019
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Welcome, @ww9rivers and thanks for the pull request. This change should have no effect due to the way the event loop works. Without a test that shows otherwise, I don't think this code should change.

@Trott Trott dismissed their stale review December 17, 2019 05:40

related issue comments

@Trott
Copy link
Member

Trott commented Dec 17, 2019

I see in the related issue, you say this fixed the immediate problem. That is...surprising. (Regardless, we do need a test for this?)

@Trott
Copy link
Member

Trott commented Dec 18, 2019

@jasnell I'm confused. AFAICT, this would seem to basically be a no-op and shouldn't be expected to fix the cited issue. What am I missing?

@richardlau
Copy link
Member

richardlau commented Dec 18, 2019

@ww9rivers Thank you for your contribution. Unfortunately I'm going to close this for the reasons mentioned by @Trott above, and also because this pull request is targetting v12.13.2-proposal when pull requests should target master (or, if not applicable to master, one of the *-staging branches).

@richardlau richardlau closed this Dec 18, 2019
@ww9rivers
Copy link
Author

Sorry, I didn't watch my emails on this. I'll try again later.

One question: when I add a test for the issue, do I add it in the repo and include it in the pull-request?

Thanks! (My first time contributing to the project, your help would be much appreciated.)

@Trott
Copy link
Member

Trott commented Dec 19, 2019

One question: when I add a test for the issue, do I add it in the repo and include it in the pull-request?

Yes.

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.

5 participants