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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

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

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

@evanlucas
Copy link
Contributor

@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

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

@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

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