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

repl: Failed to save editor mode text in .save command #8145

Closed
wants to merge 3 commits into from

Conversation

princejwesley
Copy link
Contributor

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

repl

Description of change

Fixes: #8142

Note: This pull request depends on #8143 to merge in order to pass the test.

@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Aug 17, 2016

putIn.run([`.save ${saveFileName}`]);
replServer.close();
assert.equal(fs.readFileSync(saveFileName, 'utf8'), `${cmds.join('\n')}\n`);
Copy link
Contributor

Choose a reason for hiding this comment

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

assert.strictEqual() please.

@jasnell
Copy link
Member

jasnell commented Aug 17, 2016

LGTM with green CI

@addaleax addaleax added the blocked PRs that are blocked by other issues or PRs. label Aug 18, 2016
@Fishrock123 Fishrock123 added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 19, 2016
@Fishrock123
Copy link
Contributor

sounds like a tiny feature to me

if you think otherwise, remove the label I guess

@princejwesley princejwesley removed the blocked PRs that are blocked by other issues or PRs. label Aug 19, 2016
@princejwesley
Copy link
Contributor Author

@addaleax
Copy link
Member

CI is green except for one build bot failure.

LGTM, and I think this can pass as as semver-patch change.

@jasnell
Copy link
Member

jasnell commented Aug 19, 2016

I'd say semver-patch. It seems obvious to me that this should have worked before but it was just missed.

@addaleax addaleax removed the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 19, 2016
@princejwesley
Copy link
Contributor Author

@addaleax Is it good for merging or one more attempt for CI green?

@addaleax
Copy link
Member

@princejwesley I think this is good to go. 👍

princejwesley added a commit that referenced this pull request Aug 20, 2016
Fixes: #8142
PR-URL: #8145
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@princejwesley
Copy link
Contributor Author

Landed in f6a7434

@bnoordhuis
Copy link
Member

The status line for this commit is not great. Its goal is to inform the reader what changed and why but repl: Failed to save editor mode text in .save doesn't do that, IMO - it only hints vaguely at a bug somewhere.

The lack of commit log exacerbates it. The only way to divine what really changed is to look at the diff.

@princejwesley
Copy link
Contributor Author

princejwesley commented Aug 21, 2016

@bnoordhuis Shall I amend the commit with below description ?

.save command is not capturing code executed in .editor mode. This PR addresses it

Sorry, I should have added it in the first place

@bnoordhuis
Copy link
Member

Too late now, we're well outside the 10 minute rule. :-)

@yorkie
Copy link
Contributor

yorkie commented Aug 21, 2016

Introducing a rebasing and pre-review before landing might avoid this kind of issue again?

evanlucas pushed a commit that referenced this pull request Aug 24, 2016
Fixes: #8142
PR-URL: #8145
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
evanlucas added a commit that referenced this pull request Aug 24, 2016
Notable changes:

* **buffer**: Fix regression introduced in v6.4.0 that prevented .write() at buffer end (Anna Henningsen) #8154
* **inspector**:
  * fix inspector hang while disconnecting (Aleksei Koziatinskii) #8021
  * add support for uncaught exception (Aleksei Koziatinskii) #8043
* **meta**: add @joshgav to collaborators (Josh Gavant) #8146
* **repl**: Fix saving editor mode text in `.save` (Prince J Wesley) #8145
* ***Revert*** "**repl,util**: insert carriage returns in output" (Evan Lucas) #8143
evanlucas added a commit that referenced this pull request Aug 26, 2016
Notable changes:

* **buffer**: Fix regression introduced in v6.4.0 that prevented .write() at buffer end (Anna Henningsen) #8154
* **deps**: update V8 to 5.1.281.75 (Ali Ijaz Sheikh) #8054
* **inspector**:
  * fix inspector hang while disconnecting (Aleksei Koziatinskii) #8021
  * add support for uncaught exception (Aleksei Koziatinskii) #8043
* **repl**: Fix saving editor mode text in `.save` (Prince J Wesley) #8145
* ***Revert*** "**repl,util**: insert carriage returns in output" (Evan Lucas) #8143

PR-URL: #8253
evanlucas added a commit that referenced this pull request Aug 26, 2016
Notable changes:

* **buffer**: Fix regression introduced in v6.4.0 that prevented .write() at buffer end (Anna Henningsen) #8154
* **deps**: update V8 to 5.1.281.75 (Ali Ijaz Sheikh) #8054
* **inspector**:
  * fix inspector hang while disconnecting (Aleksei Koziatinskii) #8021
  * add support for uncaught exception (Aleksei Koziatinskii) #8043
* **repl**: Fix saving editor mode text in `.save` (Prince J Wesley) #8145
* ***Revert*** "**repl,util**: insert carriage returns in output" (Evan Lucas) #8143

PR-URL: #8253
evanlucas added a commit that referenced this pull request Aug 26, 2016
Notable changes:

* **buffer**: Fix regression introduced in v6.4.0 that prevented .write() at buffer end (Anna Henningsen) #8154
* **deps**: update V8 to 5.1.281.75 (Ali Ijaz Sheikh) #8054
* **inspector**:
  * fix inspector hang while disconnecting (Aleksei Koziatinskii) #8021
  * add support for uncaught exception (Aleksei Koziatinskii) #8043
* **repl**: Fix saving editor mode text in `.save` (Prince J Wesley) #8145
* ***Revert*** "**repl,util**: insert carriage returns in output" (Evan Lucas) #8143

PR-URL: #8253
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

repl: editor mode text not saved in .save command
8 participants