FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: allow combination of -i and -e cli flags by Trott · Pull Request #5655 · nodejs/node · GitHub

/ node Public

src: allow combination of -i and -e cli flags - #5655

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:ie
ConversationCommits (1)ChecksFiles changed
Closed

src: allow combination of -i and -e cli flags#5655
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:ie

Conversation

Trott commented Mar 11, 2016

Copy link
Copy Markdown
Member
  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?

Affected core subsystem(s)

repl

Description of change

If both -i and -e flags are specified, do not ignore the -i. Instead,
launch the interactive REPL and start by evaluating the passed string.

Fixes: #1197

src: allow combination of -i and -e cli flags
346a00d 
If both -i and -e flags are specified, do not ignore the -i. Instead,
launch the interactive REPL and start by evaluating the passed string.

Fixes: nodejs#1197
Trott added repl Issues and PRs related to the REPL subsystem. semver-minor PRs that contain new features and should be released in the next minor version. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 11, 2016
Trott mentioned this pull request Mar 11, 2016
Closed

Trott commented Mar 11, 2016

Copy link
Copy Markdown
Member Author

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

jasnell commented Mar 11, 2016

Copy link
Copy Markdown
Member

LGTM

Fishrock123 commented Mar 11, 2016

Copy link
Copy Markdown
Contributor

So... what if someone passes -ie script? ... we really need a proper options parser rather than these hacks. :/

evanlucas commented Mar 11, 2016

Copy link
Copy Markdown
Contributor

@Fishrock123 I'm working on it. Hopefully will have something soon

Trott commented Mar 11, 2016

Copy link
Copy Markdown
Member Author

So... what if someone passes -ie script? ... we really need a proper options parser rather than these hacks. :/

Not sure if the "these hacks" refers to the parser (which is untouched by this PR) or the code here in this PR, so just in case:

  • Yes, proper options parsing would be great.
  • Fortunately, that's a distinct issue that can (and should) be handled separately from this.

This PR changes the handling of already-parsed options in src/node.js after they have been set by the parsing in src/node.cc.

The parsing fix would have to happen in node.cc and is independent from this change. (Parsing fix: "Let me look at argv and set the internal options properties accordingly." This fix: "Hmm, looking at how the internal options properties are set, you set these two options. I should really handle both of them rather than ignoring one. I'm not looking at the command line argv options myself at all. What? There's a command line involved here? I had no idea. Decoupling for the win.")

Fishrock123 commented Mar 11, 2016

Copy link
Copy Markdown
Contributor

Sure, but if we support this, we should really also support -ie with it..

Trott commented Mar 11, 2016

Copy link
Copy Markdown
Member Author

Sure, but if we support this, we should really also support -ie with it..

Agreed. Again, I'm not sure if you're arguing for that change to be bundled with this one, so just in case: That change is outside the scope of this change. That change would address a problem we already have, not a problem introduced by this change. Currently, node -i -e something is treated as node -e while node -ie something returns bad option and an error status code.

Trott commented Mar 11, 2016

Copy link
Copy Markdown
Member Author

Another reason for landing this before the improved parsing lands: Give the functionality to the people (or at least the one person) who want it. Sure, for a while they'll have to know to use -i -e and not -ie, but surely that's better than not having the functionality at all.

(On the other hand: The math changes depending on how soon we can expect better parsing. Like, if @evanlucas is going to submit a PR for it this week, then waiting is no big deal.)

evanlucas commented Mar 11, 2016

Copy link
Copy Markdown
Contributor

@Trott I am actively working on it, but I doubt it will be this week. Maybe 2 weeks or something like that?

Fishrock123 commented Mar 11, 2016

Copy link
Copy Markdown
Contributor

eh sure, lgtm if CI is happy

Trott added a commit to Trott/io.js that referenced this pull request Mar 16, 2016
src: allow combination of -i and -e cli flags
ba16a12 
If both -i and -e flags are specified, do not ignore the -i. Instead,
launch the interactive REPL and start by evaluating the passed string.

Fixes: nodejs#1197
PR-URL: nodejs#5655
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Trott commented Mar 16, 2016

Copy link
Copy Markdown
Member Author

Landed in ba16a12

Trott closed this Mar 16, 2016
evanlucas pushed a commit that referenced this pull request Mar 16, 2016
src: allow combination of -i and -e cli flags
4ecd996 
If both -i and -e flags are specified, do not ignore the -i. Instead,
launch the interactive REPL and start by evaluating the passed string.

Fixes: #1197
PR-URL: #5655
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
evanlucas added a commit that referenced this pull request Mar 16, 2016
2016-03-16, Version 5.9.0 (Stable)
a08ac52 
Notable changes:

* **governance**: The following members have been added as
collaborators:
  - Andreas Madsen (@AndreasMadsen)
  - Benjamin Gruenbaum (@benjamingr)
  - Claudio Rodriguez (@claudiorodriguez)
  - Glen Keane (@thekemkid)
  - Jeremy Whitlock (@whitlockjc)
  - Matt Loring (@matthewloring)
  - Phillip Johnsen (@phillipj)
* **lib**: copy arguments object instead of leaking it (Nathan Woltman)
#4361
* **src**: allow combination of -i and -e cli flags (Rich Trott)
#5655
* **zlib**: add support for concatenated members (Kári Tristan
Helgason) #5120

PR-URL: #5702
evanlucas mentioned this pull request Mar 16, 2016
Merged
evanlucas added a commit that referenced this pull request Mar 16, 2016
2016-03-16, Version 5.9.0 (Stable)
79cb1a9 
Notable changes:

* **contextify**: Fixed a memory consumption issue related to heavy use
of `vm.createContext` and `vm.runInNewContext`. (Ali Ijaz Sheikh)
#5392
* **governance**: The following members have been added as
collaborators:
  - Andreas Madsen (@AndreasMadsen)
  - Benjamin Gruenbaum (@benjamingr)
  - Claudio Rodriguez (@claudiorodriguez)
  - Glen Keane (@thekemkid)
  - Jeremy Whitlock (@whitlockjc)
  - Matt Loring (@matthewloring)
  - Phillip Johnsen (@phillipj)
* **lib**: copy arguments object instead of leaking it (Nathan Woltman)
#4361
* **src**: allow combination of -i and -e cli flags (Rich Trott)
#5655
* **v8**: backport fb4ccae from v8 upstream (Vladimir Krivosheev) #4231
  -  breakout events from v8 to offer better support for external
     debuggers
* **zlib**: add support for concatenated members (Kári Tristan
Helgason) #5120

PR-URL: #5702
evanlucas added a commit that referenced this pull request Mar 16, 2016
2016-03-16, Version 5.9.0 (Stable)
e7039db 
Notable changes:

* **contextify**: Fixed a memory consumption issue related to heavy use
of `vm.createContext` and `vm.runInNewContext`. (Ali Ijaz Sheikh)
#5392
* **governance**: The following members have been added as
collaborators:
  - Andreas Madsen (@AndreasMadsen)
  - Benjamin Gruenbaum (@benjamingr)
  - Claudio Rodriguez (@claudiorodriguez)
  - Glen Keane (@thekemkid)
  - Jeremy Whitlock (@whitlockjc)
  - Matt Loring (@matthewloring)
  - Phillip Johnsen (@phillipj)
* **lib**: copy arguments object instead of leaking it (Nathan Woltman)
#4361
* **src**: allow combination of -i and -e cli flags (Rich Trott)
#5655
* **v8**: backport fb4ccae from v8 upstream (Vladimir Krivosheev) #4231
  -  breakout events from v8 to offer better support for external
     debuggers
* **zlib**: add support for concatenated members (Kári Tristan
Helgason) #5120

PR-URL: #5702
Trott deleted the ie branch January 13, 2022 22:42
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

lib / src Issues and PRs related to general changes in the lib or src directory. repl Issues and PRs related to the REPL subsystem. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

iojs -i does nothing when -e is used

4 participants

Back | FazBrowse Home | New Git URL