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.

Sign up for GitHub

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

Jump to bottom

Wip/remove symbolic operators #931

Merged
merged 12 commits into from
Jan 25, 2017
Merged

Wip/remove symbolic operators #931

merged 12 commits into from
Jan 25, 2017

Conversation

muuki88
Copy link
Contributor

@muuki88 muuki88 commented Jan 16, 2017

No description provided.

@muuki88
Copy link
Contributor Author

muuki88 commented Jan 21, 2017

@eed3si9n / @dwijnand could one of you have a short look if the migration looks fine so far?

The only thing I stumbled upon was the ?? operator. You have an insights on this one?

@muuki88 muuki88 added the project General project issues label Jan 21, 2017
muuki88
muuki88 commented Jan 21, 2017
View reviewed changes
src/main/scala/com/typesafe/sbt/packager/archetypes/scripts/BashStartScriptPlugin.scala
@@ -55,8 +55,8 @@ object BashStartScriptPlugin extends AutoPlugin {
bashScriptDefines ++= bashScriptExtraDefines.value,
bashScriptReplacements := generateScriptReplacements(bashScriptDefines.value),
// Create a bashConfigLocation if options are set in build.sbt
bashScriptConfigLocation <<= bashScriptConfigLocation ?? Some(appIniLocation),
bashScriptEnvConfigLocation <<= bashScriptEnvConfigLocation ?? None,
bashScriptConfigLocation := (bashScriptConfigLocation ?? Some(appIniLocation)).value,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't replace the ?? by simply using

bashScriptConfigLocation := Some(appIniLocation)

Copy link
Member

Choose a reason for hiding this comment

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

What's the warning on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't seen any and haven't found anything in the migration guide.
However I haven't seen this operator anywhere else yet, so I was a bit curious :)

@eed3si9n
Copy link
Member

I only glanced at the changes, but they look ok. The tricky bits that the compiler might not catch is input tasks with Unit, but you don't have those keys except for in the scripted tests.

@muuki88
Copy link
Contributor Author

muuki88 commented Jan 21, 2017

Thanks @eed3si9n for the fast reaction.

@muuki88 muuki88 merged commit 9fa3951 into master Jan 25, 2017
@muuki88 muuki88 deleted the wip/remove-symbolic-operators branch January 25, 2017 21:02
@muuki88 muuki88 mentioned this pull request Jan 25, 2017
Closed
@dwijnand dwijnand removed their assignment Apr 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eed3si9n eed3si9n eed3si9n left review comments

Assignees

@eed3si9n eed3si9n

Labels
project General project issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@muuki88 @eed3si9n @dwijnand