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.

Sign up for GitHub

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

Jump to bottom

[v6.x backport] repl: refactor LineParser implementation #15773

Closed
wants to merge 1 commit into from
Closed

[v6.x backport] repl: refactor LineParser implementation #15773

wants to merge 1 commit into from

Conversation

lance
Copy link
Member

@lance lance commented Oct 4, 2017

Backporting this PR to address #15704. In addition to the basic backport, I have added a test test/parallel/test-repl-multi-line-templates.js to test for the issue noted in the issue above.

Original pull request: #6171

Original commit message:

Move the core logic from LineParser should fail handling into the
recoverable error check for the REPL default eval.

PR-URL: #6171
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Michaël Zasso targos@protonmail.com

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

repl

@lance
repl: refactor LineParser implementation
00be0c3 
Move the core logic from `LineParser` should fail handling into the
recoverable error check for the REPL default eval.

PR-URL: #6171
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@nodejs-github-bot nodejs-github-bot added repl Issues and PRs related to the REPL subsystem. v6.x labels Oct 4, 2017
@lance
Copy link
Member Author

lance commented Oct 4, 2017

/cc @Fishrock123 @MylesBorins

This supercedes the PR submitted earlier.

@lance
Copy link
Member Author

lance commented Oct 4, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/10406/

@lance
Copy link
Member Author

lance commented Oct 9, 2017

Lots of code-and-learn PRs to deal with, I know. Just keeping this visible. Ping @nodejs/backporting

@MylesBorins
Copy link
Contributor

@lance the original PR was Semver-Major, how has this changed to make it Semver-Patch?

@lance
Copy link
Member Author

lance commented Oct 10, 2017

@MylesBorins to be honest, I'm not sure how it should be handled. My initial reasoning for doing the backport is that it fixes #15704, which is an issue in 6.x.

If I'm reading git history/blame correctly (and I may not be), editor mode support landed in master after the line parser refactor implementation. However, when editor mode was backported to v6.x the lineParser refactoring had not been backported, so the fix noted above didn't come along for the ride.

So, I honestly don't know what the answer is. But it seems like this would be the most straightforward way to implement the fix. I initially began a PR that just pulled some relevant bits from the lineParser refactoring. But had the impression that this was not the best approach to take, so I did a full backport.

I'm ok with either approach, and would be happy to close this and reopen the former if that makes more sense.

@MylesBorins
Copy link
Contributor

/cc @nodejs/lts to chime in

If we can land this as semver patch we can get it in the next release cycle, so no rush

@gibfahn
Copy link
Member

gibfahn commented Oct 16, 2017

However, when editor mode was backported to v6.x the lineParser refactoring had not been backported, so the fix noted above didn't come along for the ride.

Assuming I'm understanding this correctly, this is a fix for #15704, in which case I'd be fine with landing it. If it's not urgent there's no reason not to wait till next release though.

@lance so to your knowledge this change itself isn't semver-major?

@lance
Copy link
Member Author

lance commented Oct 16, 2017

@gibfahn it doesn't appear to be semver-major to me. To be honest, I'm not sure why the original PR was labeled that way, since it was simply an internal refactoring of LineParser and should not have affected the user experience in any way. I believe @Fishrock123 tagged it that way, so maybe he can shed some light on the thinking there.

But to be clear, yes, this is a fix for #15704.

@lance
Copy link
Member Author

lance commented Oct 25, 2017 •
edited
Loading

CI2: https://ci.nodejs.org/job/node-test-pull-request/10972/

Update new CI looks ok in spite of some unrelated raspberry pi test failures.

@lance
Copy link
Member Author

lance commented Oct 31, 2017 •
edited
Loading

Hmm - CI failed badly. Odd. Trying again: https://ci.nodejs.org/job/node-test-pull-request/11119/

Update: An unrelated Windows failure, and a couple of the regular raspberry pi failures.

@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 14, 2017 •
edited
Loading

@nodejs/lts I've landed this in 6ca3640
please lmk if this should be reverted

@MylesBorins
Copy link
Contributor

@jasnell @Fishrock123 you two originally labelled this change semver major. do you stand by that original review? should I back this out?

MylesBorins pushed a commit that referenced this pull request Nov 14, 2017
@lance @MylesBorins
repl: refactor LineParser implementation
6ca3640 
Move the core logic from `LineParser` should fail handling into the
recoverable error check for the REPL default eval.

Fixes: #15704
Backport-PR-URL: #15773
PR-URL: #6171
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins MylesBorins reopened this Nov 21, 2017
@MylesBorins
Copy link
Contributor

Hey @lance I had to back this out of v6.x as it is breaking coffeescript 😢

@MylesBorins MylesBorins mentioned this pull request Nov 22, 2017
Merged
@lance
Copy link
Member Author

lance commented Nov 22, 2017

@MylesBorins ok - what's next? I'd like to find a fix for #15704 but I guess this won't be it.

@MylesBorins
Copy link
Contributor

@lance if we can find a way to do this that doesn't break ecosystem modules we can land it

@lance
Copy link
Member Author

lance commented Nov 27, 2017

@MylesBorins is there a known way to reproduce the errors that were seen with coffeescript?

@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 27, 2017 via email

@BridgeAR
Copy link
Member

How shall we continue here?

@lance
Copy link
Member Author

lance commented Feb 1, 2018

@BridgeAR I'm sorry I have been quite consumed with Red Hat related work lately and have not had an opportunity to get back to this. As mentioned in the initial comment, it is meant to address #15704. I do not have a good sense of how important this issue is and whether a backport is really necessary. If so, I assume we'd want it before active LTS ends in April?

@BridgeAR
Copy link
Member

BridgeAR commented Feb 1, 2018

I personally would say it is not as important but I do not have a strong opinion in this case.

@MylesBorins what do you think?

@MylesBorins
Copy link
Contributor

As the error only exists in the repl and I have not seen other people report the issue, I think it is reasonable to close this if @lance doesn't have time to dig in

@lance
Copy link
Member Author

lance commented Feb 5, 2018

OK - I will close this for now. If I get a little breathing room to pick it up again I may.

@lance lance closed this Feb 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lance @MylesBorins @gibfahn @BridgeAR @nodejs-github-bot