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.

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

doc: update REPL documentation to instantiate the REPL #30928

Closed
wants to merge 2 commits into from

Conversation

@BridgeAR
Copy link
Member

BridgeAR commented Dec 13, 2019

The documentation currently states that it's not intended to directly
instantiate REPL instances. It is unknown for what reason that's
recommended as it does seem a proper way to handle new REPL instances.

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
The documentation currently states that it's not intended to directly
instantiate REPL instances. It is unknown for what reason that's
recommended as it does seem a proper way to handle new REPL instances.
@BridgeAR BridgeAR requested review from jasnell and Trott Dec 13, 2019
@targos
targos approved these changes Dec 13, 2019
@ZYSzys
ZYSzys approved these changes Dec 13, 2019
@Trott
Trott approved these changes Dec 13, 2019
doc/api/repl.md Outdated Show resolved Hide resolved
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 13, 2019

Bottom reference needs to be corrected but other than that, looks good to me.

@BridgeAR

This comment has been minimized.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Dec 14, 2019

@BridgeAR Using repl.start() and new REPLServer() are not equivalent, though, and the documentation seems to imply otherwise? Can you also change the code to work the same in both cases, and then add that to the YAML changelog?

@addaleax addaleax removed the author ready label Dec 14, 2019
@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 14, 2019

@addaleax I'll add a note about the behavior difference. The behavior of .start is somewhat special though: it just adds the very first repl instance as property on the export. I am not certain what reason there was to add this. The commit that adds this property does not indicate any reasons for it either (the description and main code change is about a different feature).
What do you think about checking the usage with Gzemnid and citgm and to deprecate that property if it's not frequently used? My guess is that this property is not used at all.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Dec 14, 2019

@BridgeAR It adds the repl to replMap. That’s the more important bit here imo.

What do you think about checking the usage with Gzemnid and citgm and to deprecate that property if it's not frequently used? My guess is that this property is not used at all.

I think both tools would be useless in this case, and I see no reason to deprecate. repl.repl is the way to access the current REPL instance from the REPL itself, and I expect programmatic usage to be minimal.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 15, 2019

@addaleax I somehow did not really think about accessing the current REPL. That is indeed something we should not remove. I think we should move that into the constructor and either only make this accessible in the standalone repl or each instance instead of exporting the very first repl instance that is created with .start() in each repl instance.
I'll mark this PR as blocked and open a PR to implement the exposure when using it as standalone REPL.

It adds the repl to replMap

I removed replMap in a recent commit. So that's not an issue anymore.

@BridgeAR BridgeAR added the blocked label Dec 15, 2019
@BridgeAR BridgeAR mentioned this pull request Dec 15, 2019
4 of 4 tasks complete
@BridgeAR BridgeAR removed the blocked label Jan 3, 2020
BridgeAR added a commit that referenced this pull request Jan 3, 2020
The documentation currently states that it's not intended to directly
instantiate REPL instances. It is unknown for what reason that's
recommended as it does seem a proper way to handle new REPL instances.

PR-URL: #30928
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Jan 3, 2020

Landed in 84b15eb 🎉

@BridgeAR BridgeAR closed this Jan 3, 2020
targos added a commit that referenced this pull request Jan 6, 2020
The documentation currently states that it's not intended to directly
instantiate REPL instances. It is unknown for what reason that's
recommended as it does seem a proper way to handle new REPL instances.

PR-URL: #30928
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
targos added a commit that referenced this pull request Jan 14, 2020
The documentation currently states that it's not intended to directly
instantiate REPL instances. It is unknown for what reason that's
recommended as it does seem a proper way to handle new REPL instances.

PR-URL: #30928
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR deleted the BridgeAR:update-repl-start-doc branch Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.