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-5479 deprecate DelayedInit outside of App #3563

Merged
merged 2 commits into from Feb 22, 2014
Merged

Conversation

@adriaanm
Copy link
Member

adriaanm commented Feb 20, 2014

DelayedInit's semantics are way too surprising.

For example, it delays initialization of fields,
so that fields on objects that extend App
(which extends DelayedInit) are not initialized
until the main method is called.

For more details and a proposed alternative,
see https://issues.scala-lang.org/browse/SI-4330?jql=labels%20%3D%20delayedinit%20AND%20resolution%20%3D%20unresolved.

Support for App will continue -- we'll special case it.

review by @gkossakowski

DelayedInit's semantics are way too surprising.

For example, it delays initialization of fields,
so that fields on objects that extend `App`
(which `extends DelayedInit`) are not initialized
until the `main` method is called.

For more details and a proposed alternative,
see https://issues.scala-lang.org/browse/SI-4330?jql=labels%20%3D%20delayedinit%20AND%20resolution%20%3D%20unresolved.

Support for `App` will continue -- we'll special case it.
@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Feb 20, 2014

Should I retract my comment on

http://stackoverflow.com/a/21885076/1296806

Maybe it's the cargo-cultist in me talking, but this strikes me as a nice application of the DelayedInit magic.

I also mentioned the downside of deprecation.

@adriaanm

This comment has been minimized.

Copy link
Member Author

adriaanm commented Feb 20, 2014

Would SI-4330's postCreate cover that use case? I must admit I'm getting a bit bleary-eyed from deprecating all this dusty code, so I didn't look very closely.

@adriaanm adriaanm added the tested label Feb 20, 2014
@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Feb 20, 2014

That makes me want to run over with cucumber and avocado facial mask, but the use case was to wrap init code in breakable; but you could imagine anything that looks like the App use case, namely, I have init code that I want to handle as an ordinary block.

Famously, REPL code translates to members but block semantics is more intuitive. I wonder if REPL templates should be DelayedInit? I haven't investigated (yet).

Furthermore, it was hard enough when my daughter was singing all the songs from Frozen all day long, over and over, but she heard the Pink duet Give Me a Reason in the car and then found it on youtube so now she's singing that over and over and now it's going through my head. Does DelayedInit address that use case? She's not supposed to be searching youtube by herself.

@adriaanm

This comment has been minimized.

Copy link
Member Author

adriaanm commented Feb 20, 2014

That's one initiation I imagine every parent would like to delay.

@@ -43,6 +43,7 @@ package scala
*
* @author Martin Odersky
*/
@deprecated("DelayedInit semantics can be surprising.\n(For details and a proposed alternative, see https://issues.scala-lang.org/browse/SI-4330?jql=labels%20%3D%20delayedinit%20AND%20resolution%20%3D%20unresolved. Support for `App` will continue.)", "2.11.0")

This comment has been minimized.

Copy link
@xeno-by

xeno-by Feb 20, 2014

Member

The URL seems a bit longer than necessary.

This comment has been minimized.

@gkossakowski

This comment has been minimized.

Copy link
Member

gkossakowski commented Feb 20, 2014

I'd prefer if we used saved search which generates less noisy URL: https://issues.scala-lang.org/browse/SI-4330?filter=12418

Other than that it looks great.

@adriaanm

This comment has been minimized.

Copy link
Member Author

adriaanm commented Feb 20, 2014

The problem with saved searches is that they can be deleted accidentally.

@gkossakowski

This comment has been minimized.

Copy link
Member

gkossakowski commented Feb 20, 2014

Every problem in computer science can be solved with another layer of indirection. How about linking to release notes where we would have a paragraph or two motivating for that deprecation and from there link to JIRA tickets?

@adriaanm

This comment has been minimized.

@gkossakowski

This comment has been minimized.

Copy link
Member

gkossakowski commented Feb 20, 2014

Ah, I forgot that our release notes include the date so we can't predict the URL :-/
I guess linking to github is not a bad option.

@adriaanm

This comment has been minimized.

Copy link
Member Author

adriaanm commented Feb 20, 2014

I included a draft of the blurb in the release notes already: scala/make-release-notes#50

@adriaanm

This comment has been minimized.

Copy link
Member Author

adriaanm commented Feb 20, 2014

Pushed a new commit.

@adriaanm adriaanm removed the tested label Feb 20, 2014
@@ -4,9 +4,13 @@ delayed-init-ref.scala:17: warning: Selecting value vall from object O, which ex
delayed-init-ref.scala:19: warning: Selecting value vall from object O, which extends scala.DelayedInit, is likely to yield an uninitialized value
println(vall) // warn
^
delayed-init-ref.scala:28: warning: trait DelayedInit in package scala is deprecated: DelayedInit semantics can be surprising.
(For details and a proposed alternative, see https://issues.scala-lang.org/browse/SI-4330?jql=labels%20%3D%20delayedinit%20AND%20resolution%20%3D%20unresolved. Support for `App` will continue.)

This comment has been minimized.

Copy link
@gkossakowski

gkossakowski Feb 20, 2014

Member

You forgot to update the test so it will fail.

This comment has been minimized.

Copy link
@adriaanm

adriaanm Feb 20, 2014

Author Member

thanks -- not a great idea to code while getting ready for the airport

@adriaanm

This comment has been minimized.

Copy link
Member Author

adriaanm commented Feb 20, 2014

check file updated :)

@adriaanm

This comment has been minimized.

Copy link
Member Author

adriaanm commented Feb 20, 2014

/me afk

@adriaanm adriaanm added tested and removed tested labels Feb 20, 2014
@gkossakowski

This comment has been minimized.

Copy link
Member

gkossakowski commented Feb 22, 2014

LGTM

gkossakowski added a commit that referenced this pull request Feb 22, 2014
SI-5479 deprecate DelayedInit outside of App
@gkossakowski gkossakowski merged commit b0dcf79 into scala:master Feb 22, 2014
1 check passed
1 check passed
default pr-scala Took 74 min.
Details
@etorreborre

This comment has been minimized.

Copy link

etorreborre commented Mar 6, 2014

Just FYI, specs2 is using DelayedInit to enable this use case:

"this url must be ok" >> new Context {
   // whatever needs to be done
   // and potentially throws Exceptions
}

I need Context to extend DelayedInit in order to case test failures (and possibly trigger before/after behaviour). I hope there will be an alternative to that when DelayedInit disappears.

@adriaanm

This comment has been minimized.

Copy link
Member Author

adriaanm commented Mar 6, 2014

Thanks for the use case. The primary goal of the deprecation is to warn
people. We won't actually remove it until we've provided an alternative
that covers usages like these.

On Wednesday, March 5, 2014, Eric Torreborre notifications@github.com
wrote:

Just FYI, specs2 is using DelayedInit to enable this use case:

"this url must be ok" >> new Context {
// whatever needs to be done
// and potentially throws Exceptions}

I need Context to extend DelayedInit in order to case test failures (and
possibly trigger before/after behaviour). I hope there will be an
alternative to that when DelayedInit disappears.

Reply to this email directly or view it on GitHubhttps://github.com//pull/3563#issuecomment-36823400
.

@adriaanm adriaanm deleted the adriaanm:t5479 branch Mar 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.