repl: display error message when loading directory by princejwesley · Pull Request #4170 · nodejs/node

princejwesley · 2015-12-06T09:33:03Z

When loading directory instead of file, no error message
is displayed. Its good to display error message for
this scenario.

Before:

 > .load /Users/princejohnwesley/Projects/Playground/Node
>

After:

 > .load /Users/princejohnwesley/Projects/Playground/Node
Failed to load:/Users/princejohnwesley/Projects/Playground/Node is not a file
>

JungMinu · 2015-12-06T10:29:46Z

maybe is not a file -> is not a valid file

@JungMinu Yes.

princejwesley · 2015-12-08T09:34:01Z

@JungMinu Updated.

JungMinu · 2015-12-09T15:18:22Z

/cc @evanlucas @thefourtheye PTAL :)

jasnell · 2015-12-09T17:12:36Z

Semver-major due to the added throw

jasnell · 2015-12-09T17:13:14Z

LGTM

JungMinu · 2015-12-09T23:33:56Z

LGTM

bnoordhuis · 2015-12-10T13:42:55Z

Can you add a regression test to test/parallel/test-repl-.save.load.js?

princejwesley · 2015-12-10T15:12:43Z

@bnoordhuis Sure. I'll add and update here

When loading directory instead of file, no error message is displayed. Its good to display error message for this scenario. Before: > .load /Users/princejohnwesley/Projects/Playground/Node > After: > .load /Users/princejohnwesley/Projects/Playground/Node Failed to load:/Users/princejohnwesley/Projects/Playground/Node is not a valid file >

princejwesley · 2015-12-10T16:20:21Z

@bnoordhuis Test added

bnoordhuis · 2015-12-10T18:21:50Z

LGTM, thanks. CI: https://ci.nodejs.org/job/node-test-pull-request/975/

bnoordhuis · 2015-12-10T18:22:19Z

@JungMinu I see you've assigned this to yourself. Feel free to land it when the CI checks out.

JungMinu · 2015-12-11T01:10:12Z

CI is happy except for unrelated Windows test :)

When loading directory instead of file, no error message is displayed. It's good to display error message for this scenario. PR-URL: #4170 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>

JungMinu · 2015-12-11T01:23:25Z

Thanks, landed in aad6b9f

Fishrock123 · 2015-12-11T16:20:26Z

@jasnell how is this semver-major?

jasnell · 2015-12-11T17:04:14Z

@Fishrock123 ... precautionary based on the new throw. If folks are
comfortable with it as semver-minor then I'm good too.
On Dec 11, 2015 8:20 AM, "Jeremiah Senkpiel" notifications@github.com
wrote:

@jasnell https://github.com/jasnell how is this semver-major?
—
Reply to this email directly or view it on GitHub
#4170 (comment).

ChALkeR · 2015-12-11T17:11:09Z

@jasnell Is there a new throw?

jasnell · 2015-12-11T17:17:33Z

Sorry, not new throw... sigh, I'm doing too many things at once. I meant the new output message. While it's not technically a throw, it is a new output indicating that the operation failed. The ctc had decided to treat changes to error messages as semver-major... this one comes across as a grey area to me so to be safe I flagged it. Like I said tho, I'm perfectly fine with removing that if others are -- I may just be being overly conservative on it :-)

ChALkeR · 2015-12-11T17:19:33Z

The ctc had decided to treat changes to error messages as semver-major...

Ah, that makes sense then, thanks for the explanation! =)

Fishrock123 · 2015-12-11T17:26:25Z

I don't think REPL warnings count as error messages.

(We've added and changed lots of them with the repl history stuff anyways.)

jasnell · 2015-12-11T17:27:41Z

@Fishrock123 ... ok! works for me then. Removing the flag :-)

When loading directory instead of file, no error message is displayed. It's good to display error message for this scenario. PR-URL: #4170 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>

Notable changes: * assert - accommodate ES6 classes that extend Error (Rich Trott) #4166 * build - add "--partly-static" build options (Super Zheng) #4152 * deps - backport 066747e from upstream V8 (Ali Ijaz Sheikh) #4655 - backport 200315c from V8 upstream (Vladimir Kurchatkin) #4128 - upgrade libuv to 1.8.0 (Saúl Ibarra Corretgé) * docs - various updates landed in 70 different commits! * repl - attach location info to syntax errors (cjihrig) #4013 - display error message when loading directory (Prince J Wesley) #4170 * tests - various updates landed in over 50 commits * util - allow lookup of hidden values (cjihrig) #3988

Notable changes: * assert - accommodate ES6 classes that extend Error (Rich Trott) #4166 * build - add "--partly-static" build options (Super Zheng) #4152 * deps - backport 066747e from upstream V8 (Ali Ijaz Sheikh) #4655 - backport 200315c from V8 upstream (Vladimir Kurchatkin) #4128 - upgrade libuv to 1.8.0 (Saúl Ibarra Corretgé) * docs - various updates landed in 70 different commits! * repl - attach location info to syntax errors (cjihrig) #4013 - display error message when loading directory (Prince J Wesley) #4170 * tests - various updates landed in over 50 commits * util - allow lookup of hidden values (cjihrig) #3988 PR-URL: #4768

Notable changes: * assert - accommodate ES6 classes that extend Error (Rich Trott) #4166 * build - add "--partly-static" build options (Super Zheng) #4152 * deps - backport 066747e from upstream V8 (Ali Ijaz Sheikh) #4655 - backport 200315c from V8 upstream (Vladimir Kurchatkin) #4128 - upgrade libuv to 1.8.0 (Saúl Ibarra Corretgé) * docs - various updates landed in 70 different commits! * repl - attach location info to syntax errors (cjihrig) #4013 - display error message when loading directory (Prince J Wesley) #4170 * tests - various updates landed in over 50 commits * util - allow lookup of hidden values (cjihrig) #3988 PR-URL: #4768

When loading directory instead of file, no error message is displayed. It's good to display error message for this scenario. PR-URL: nodejs#4170 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>

Notable changes: * assert - accommodate ES6 classes that extend Error (Rich Trott) nodejs#4166 * build - add "--partly-static" build options (Super Zheng) nodejs#4152 * deps - backport 066747e from upstream V8 (Ali Ijaz Sheikh) nodejs#4655 - backport 200315c from V8 upstream (Vladimir Kurchatkin) nodejs#4128 - upgrade libuv to 1.8.0 (Saúl Ibarra Corretgé) * docs - various updates landed in 70 different commits! * repl - attach location info to syntax errors (cjihrig) nodejs#4013 - display error message when loading directory (Prince J Wesley) nodejs#4170 * tests - various updates landed in over 50 commits * util - allow lookup of hidden values (cjihrig) nodejs#3988 PR-URL: nodejs#4768

JungMinu added the repl Issues and PRs related to the REPL subsystem. label Dec 6, 2015

JungMinu reviewed Dec 6, 2015
View reviewed changes

princejwesley force-pushed the repl-load-dir branch 2 times, most recently from 88ebc6d to 3e03a6aCompare December 6, 2015 10:51

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 9, 2015

JungMinu self-assigned this Dec 9, 2015

princejwesley force-pushed the repl-load-dir branch from 3e03a6a to 8a6db50Compare December 10, 2015 16:18

JungMinu closed this Dec 11, 2015

jasnell removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 11, 2015

Fishrock123 mentioned this pull request Dec 15, 2015
v5.3.0 Release Proposal (Stable) #4281
Closed

rvagg mentioned this pull request Dec 17, 2015
Release 4.2.4 Planning #4321
Closed

jasnell added the lts-watch-v4.x label Dec 17, 2015

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Dec 29, 2015

MylesBorins mentioned this pull request Jan 19, 2016
V4.2.5 proposal #4768
Merged

This was referenced Jul 31, 2021
[Snyk] Upgrade rollup from 2.32.1 to 2.53.0 Zintexiss/node-22#44
Open
[Snyk] Upgrade rollup from 2.32.1 to 2.53.1 Zintexiss/node-22#45
Open
[Snyk] Upgrade rollup from 2.32.1 to 2.53.2 Zintexiss/node-22#46
Open

This was referenced Aug 12, 2021
[Snyk] Upgrade rollup from 2.32.1 to 2.53.3 Zintexiss/node-22#47
Open
[Snyk] Upgrade rollup from 2.32.1 to 2.54.0 Zintexiss/node-22#50
Open

snyk-bot mentioned this pull request Apr 30, 2023
[Snyk] Fix for 15 vulnerabilities aliscco/alisco-node#387
Open

Uh oh!

Uh oh!

Conversation

princejwesley commented Dec 6, 2015

Uh oh!

JungMinuDec 6, 2015

Choose a reason for hiding this comment

Uh oh!

princejwesleyDec 6, 2015

Choose a reason for hiding this comment

Uh oh!

princejwesley commented Dec 8, 2015

Uh oh!

JungMinu commented Dec 9, 2015

Uh oh!

jasnell commented Dec 9, 2015

Uh oh!

jasnell commented Dec 9, 2015

Uh oh!

JungMinu commented Dec 9, 2015

Uh oh!

bnoordhuis commented Dec 10, 2015

Uh oh!

princejwesley commented Dec 10, 2015

Uh oh!

princejwesley commented Dec 10, 2015

Uh oh!

bnoordhuis commented Dec 10, 2015

Uh oh!

bnoordhuis commented Dec 10, 2015

Uh oh!

JungMinu commented Dec 11, 2015

Uh oh!

JungMinu commented Dec 11, 2015

Uh oh!

Fishrock123 commented Dec 11, 2015

Uh oh!

jasnell commented Dec 11, 2015

Uh oh!

ChALkeR commented Dec 11, 2015

Uh oh!

jasnell commented Dec 11, 2015

Uh oh!

ChALkeR commented Dec 11, 2015

Uh oh!

Fishrock123 commented Dec 11, 2015

Uh oh!

jasnell commented Dec 11, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants