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

make some repl components extensible #1342

Merged
merged 12 commits into from
Jul 1, 2015
Merged

Conversation

rubanm
Copy link
Contributor

@rubanm rubanm commented Jun 27, 2015

Allows for setting custom implicits and config to load at boot time.

@@ -77,14 +82,16 @@ class ScaldingILoop
*/
override def commands: List[LoopCommand] = super.commands ++ scaldingCommands

protected def shellImports: List[String] = List(
Copy link
Collaborator

Choose a reason for hiding this comment

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

could this be moved to BaseScaldingShell so there is only one place to configure if it makes sense?

@johnynek
Copy link
Collaborator

If we could have some trait like ShellDefaults or something that has all the default constants, that seems like it would solve most of the issues, then maybe we wouldn't need to bolt them onto other code here. Would that work?

@@ -178,7 +183,8 @@ object ReplImplicits extends FieldConversions {
* @param source to convert to a Pipe.
* @return a Pipe that is the result of reading the specified Source.
*/
implicit def sourceToPipe(source: Source): Pipe = source.read(flowDef, mode)
implicit def sourceToPipe(source: Source)(implicit flowDef: FlowDef, mode: Mode): Pipe =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really don't think we need this. I think it was just cargo culted from Dsl by the (wibidata folks that started the REPL).

@johnynek
Copy link
Collaborator

if the args change makes sense, I'm happy to merge.

@rubanm
Copy link
Contributor Author

rubanm commented Jul 1, 2015

I made a few more changes and moved the prompt & imports to ScaldingILoop, so it no longer needs a reference to ScaldingShell.

The main issue is that ScaldingShell needs a reference to ReplState to mutate it (setting the mode and custom config based on command line args), and ReplState needs ScaldingShell to build the jars correctly for executing flows.

I've made ShellPipe take in the ReplState for execution. This state can be customized with different defaults. Still not completely clean but hopefully doesn't make it worse.

@johnynek
Copy link
Collaborator

johnynek commented Jul 1, 2015

I think this is an improvement.

I'm happy to merge this. @ianoc want to merge if you think this looks good?

ianoc added a commit that referenced this pull request Jul 1, 2015
make some repl components extensible
@ianoc ianoc merged commit c394a1c into twitter:develop Jul 1, 2015
@ianoc
Copy link
Collaborator

ianoc commented Jul 1, 2015

LGTM, merged

@rubanm rubanm deleted the rmonu/repl_extend branch July 1, 2015 17:37
@ianoc ianoc mentioned this pull request Aug 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants