Skip to content

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

Closed
Trott wants to merge 1 commit intonodejs:masterfrom
Trott:ie
Closed

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

Conversation

@Trott
Copy link
Member

@Trott Trott commented Mar 11, 2016

  • 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

@Trott
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 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 Trott mentioned this pull request Mar 11, 2016
Closed
@Trott
Copy link
Member Author

Trott commented Mar 11, 2016

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

@jasnell
Copy link
Member

jasnell commented Mar 11, 2016

LGTM

@Fishrock123
Copy link
Contributor

Fishrock123 commented Mar 11, 2016

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

@evanlucas
Copy link
Contributor

evanlucas commented Mar 11, 2016

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

@Trott
Copy link
Member Author

Trott commented Mar 11, 2016

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
Copy link
Contributor

Fishrock123 commented Mar 11, 2016

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

@Trott
Copy link
Member Author

Trott commented Mar 11, 2016

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
Copy link
Member Author

Trott commented Mar 11, 2016

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
Copy link
Contributor

evanlucas commented Mar 11, 2016

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

@Fishrock123
Copy link
Contributor

Fishrock123 commented Mar 11, 2016

eh sure, lgtm if CI is happy

Trott added a commit to Trott/io.js that referenced this pull request Mar 16, 2016
@Trott
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
Copy link
Member Author

Trott commented Mar 16, 2016

Landed in ba16a12

@Trott Trott closed this Mar 16, 2016
evanlucas pushed a commit that referenced this pull request Mar 16, 2016
@Trott @evanlucas
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
@evanlucas
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 evanlucas mentioned this pull request Mar 16, 2016
Merged
evanlucas added a commit that referenced this pull request Mar 16, 2016
@evanlucas
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
@evanlucas
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 Trott deleted the ie branch January 13, 2022 22:42
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

@Trott @jasnell @Fishrock123 @evanlucas