-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add -Ysafe-init option #85
Conversation
Are we good with this? |
Ping @satorg |
@satorg @armanbilge @BalmungSan How can we move forward on this? |
@@ -647,6 +650,7 @@ private[scalacoptions] trait ScalacOptions { | |||
val privateOptions: Set[ScalacOption] = ListSet( | |||
privateNoAdaptedArgs, | |||
privateKindProjector, | |||
privateSafeInit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this line enable it by default? If that's the case, I won't as it's still marked as experimental. There might be other experimental flags here that are already included by default though, I'm not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it will. Should I remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know there is a mention of the word "experimental" but it's not under the Experimental
section https://docs.scala-lang.org/scala3/reference/other-new-features/safe-initialization.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this is check so not sure if the experimental refers to the fact that it wouldn't catch all of them or if it would fail on false positives like the unused imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why it's not in the experimental section, maybe an oversight 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it will. Should I remove it?
Yes, Seth explained here why: #85 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adressed
"partial-unification", | ||
version => version.isBetween(V2_11_11, V2_13_0) | ||
) | ||
privateOption("partial-unification", version => version.isBetween(V2_11_11, V2_13_0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify: is this change supposed to be about re-formatting only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on this. If it's just reformatting please revert it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adressed
This is experimental and should not be enabled by default. In general |
LGTM now. @SethTisue, @satorg opinions? |
No description provided.