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

SI-4625 Recognize App in script #5169

Merged
merged 5 commits into from May 23, 2016
Merged

SI-4625 Recognize App in script #5169

merged 5 commits into from May 23, 2016

Conversation

som-snytt
Copy link
Contributor

Cheap name test: if the script object extends "App",
take it for a main-bearing parent.

Note that if -Xscript is not Main, the default,
then the source is taken as a snippet and there is
no attempt to locate an existing main method.

Cheap name test: if the script object extends "App",
take it for a main-bearing parent.

Note that if `-Xscript` is not `Main`, the default,
then the source is taken as a snippet and there is
no attempt to locate an existing `main` method.
@scala-jenkins scala-jenkins added this to the 2.11.9 milestone May 17, 2016
@@ -380,11 +380,15 @@ self =>
case DefDef(_, nme.main, Nil, List(_), _, _) => true
case _ => false
}
def isApp(t: Tree) = t match {
case Template(ps, _, _) => ps.exists { case Ident(x) if x.decoded == "App" => true ; case _ => false }
Copy link
Member

Choose a reason for hiding this comment

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

x == tpnme.App would be more idiomatic. That form avoids needless string creation.

Copy link
Member

Choose a reason for hiding this comment

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

or even case Ident(tpnme.App) =>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx, I grepped for it but missed it, trying to beat the evening rush home. Unless you mean add it to names.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I mean to add it.

@retronym
Copy link
Member

LGTM, nitpick aside.

@retronym
Copy link
Member

Scope creep request: Would be great if we could detect Appy modules in files with more than one definition.

% cat sandbox/test.scala && scala sandbox/test.scala
object Test {  def main(args: Array[String]) { println("!!") }}
!!

% cat sandbox/test.scala && scala sandbox/test.scala
object O
object Test {  def main(args: Array[String]) { println("!!") }}

%

@som-snytt
Copy link
Contributor Author

som-snytt commented May 17, 2016

I considered a REPLish strategy, put the entire text in a package object.

Edit: didn't do that here. But it now allows

trait X { def x = 42 }

object Main extends X with App {
  println(x)
}

Scripting knows it by name.
In an unwrapped script, where a `main` entry point is discovered
in a top-level object, retain all top-level classes.

Everything winds up in the default package.
It's pretty confusing when your script object becomes a local
and then nothing happens. Such as when you're writing a test and
it takes forever to figure out what's going on.
Fixed the warning when main module is accompanied by snippets.

Minor clean-up so even I can follow what is returned.
@som-snytt
Copy link
Contributor Author

I thought the scope creep was the guy with binoculars who peers in people's windows.

}
}

if (mainModuleName == newTermName(ScriptRunner.defaultScriptMain))
searchForMain() foreach { return _ }
Copy link
Member

Choose a reason for hiding this comment

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

wow, never seen that pattern before..!

@lrytz
Copy link
Member

lrytz commented May 23, 2016

LGTM, too

@lrytz lrytz merged commit 9a703b4 into scala:2.11.x May 23, 2016
@som-snytt
Copy link
Contributor Author

I just expected this to work, but I guess it hasn't been merged forward yet...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants