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

repl: simplify internal repl #33461

Closed
wants to merge 1 commit into from
Closed

repl: simplify internal repl #33461

wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented May 19, 2020 •
edited
Loading

repl: always check for NODE_REPL_MODE environment variable

This makes sure all REPL instances check for the NODE_REPL_MODE
environment variable in case the replMode is not passed through
as option.

At the same time this simplifies the internal REPL code significantly
and color detection is improved.

##### doc: document the --use-strict flag for the REPL

Add information that this flag is going to evaluate all code in strict
mode. Also use the regular default description as it's used in other
options.

@nodejs/repl @nodejs/documentation PTAL

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label May 19, 2020
@BridgeAR BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label May 19, 2020
@nodejs-github-bot
Copy link
Collaborator

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

devsnek
devsnek reviewed May 20, 2020
View reviewed changes
doc/api/repl.md Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@BridgeAR
Copy link
Member Author

@nodejs/repl this could use some reviews. PTAL.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 23, 2020 •
edited by BridgeAR
Loading

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

@BridgeAR
Copy link
Member Author

This could use some reviews. I am not sure whom to ping though.

@benjamingr
Copy link
Member

This generally LGTM. Maybe @addaleax since she had an opinion on the older repl PR?

I'll try checking this out and reviewing it today/tomorrow.

@addaleax
Copy link
Member

Yeah idk, I’ve seen this PR but I don’t really know how to feel about it? The commit message says what this PR does, but not why, and that might be helpful here.

To be honest, I think that’s mostly because I don’t know if this would be more helpful to – the probably very low number of – external REPL module users or not. For other modules I would consider relying on environment variables for config an antipattern, but given that the REPL is probably more often used as the main application than as part of a larger application, it might make sense here?

@BridgeAR
Copy link
Member Author

@addaleax my main goal here is to unify the behavior of our "internal" (standalone) REPL instances and instances created otherwise. The improved color detection could theoretically be handled separately.

@BridgeAR
repl: always check for NODE_REPL_MODE environment variable
c0e034c 
This makes sure all REPL instances check for the NODE_REPL_MODE
environment variable in case the `replMode` is not passed through
as option.

At the same time this simplifies the internal REPL code significantly.

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 30, 2020 •
edited by BridgeAR
Loading

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

benjamingr
benjamingr approved these changes May 30, 2020
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Actual code LGTM, not sure I understand the use case well enough but I trust you on this

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 30, 2020
@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 25, 2020 •
edited by jasnell
Loading

Running CI again since the last one was 27 days ago

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

@nodejs-github-bot
Copy link
Collaborator

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

jasnell pushed a commit that referenced this pull request Jun 25, 2020
@BridgeAR @jasnell
repl: always check for NODE_REPL_MODE environment variable
b831b08 
This makes sure all REPL instances check for the NODE_REPL_MODE
environment variable in case the `replMode` is not passed through
as option.

At the same time this simplifies the internal REPL code significantly.

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #33461
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jun 25, 2020

Landed in b831b08

@jasnell jasnell closed this Jun 25, 2020
@github-actions github-actions bot mentioned this pull request Jun 26, 2020
Open
13 tasks
@addaleax
Copy link
Member

Revert PR because this breaks the github actions build: #34058

This was referenced Jun 27, 2020
Open
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devsnek devsnek devsnek left review comments

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@targos targos Awaiting requested review from targos

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
6 participants
@BridgeAR @nodejs-github-bot @benjamingr @addaleax @jasnell @devsnek