-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
SI-7747 Support class based wrappers in REPL #3123
Conversation
I have no idea why the IDE build failed. It's probably my fault, though. First it successfully downloads the jar from maven, then it says it can't find it. Investigating. |
https://scala-webapps.epfl.ch/jenkins/job/pr-scala-integrate-ide/3919/console
Great. |
Found a couple of old floppies lying around. https://scala-webapps.epfl.ch/jenkins/job/pr-scala-integrate-ide/3920/console |
PLS REBUILD ALL |
(kitty-note-to-self: ignore 28175874) |
@adriaanm Thanks. I'm hoping this gets into the M7 pipeline by Monday's COB. Is that when the sun sets on Guam or something? I have to take another look at the test diff on the first commit -- I thought I'd fixed it. if it frustrates me, I'll have to squash the commits -- I only wanted to preserve the original commit from ScrapCodes, for the record in case it reveals something about the use case. |
…repl-class-based Refactoring to reduce the number of if-else Fix test.
Simplified the code paths to just use one of two `Wrapper` types for textual templating. Simplified the class-based template to use the same `$iw` name for the both the class and the wrapper value. In addition, the $read value is an object extending $read, instead of containing an extra instance field, which keeps paths to values the same for both templates. Both styles trigger loading the value object by referencing the value that immediately wraps the user code, although for the class style, inner vals are eager and it would suffice to load the enclosing `$read` object. The proposed template included extra vals for values imported from history, but this is not necessary since such an import is always a stable path. (Or, counter-example to test is welcome.) The test for t5148 is updated as a side effect. Probably internal APIs don't make good test subjects. Modify -Y option message.
This |
Samoan deadlines ftw. Milestone granted. |
LGTM -- thanks! |
SI-7747 Support class based wrappers in REPL
This is @ScrapCodes proposal with simplifications.
The original commits are squashed and pass tests.
The simplification removes changes to the typical $read.$iw.$iw.value paths. The same name ($read or $iw) is used for both the type of the wrapper and the value term at each level of nesting. So the path looks the same for both templates.
The same command option (-Yrepl-class-based) is used to pick one of two Wrapper subtypes. Wrapper is just a CodeAssembler. The wrapper is also passed to the imports wrapping code.
Review by @retronym