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

define def config(...) as a macro to capture the Scala identifier #113

Merged
merged 3 commits into from
Jul 10, 2017

Conversation

eed3si9n
Copy link
Member

@eed3si9n eed3si9n commented Jun 10, 2017

In preparation to forward porting sbt/sbt#3255, this changes the implementation of def config(name: String) so it captures the LHS as the id field in Configuration.
The constructor now enforces that the id (the Scala identifier for val) starts with upper case, like Compile and Test.

lazy val Compile = config("compie")

sbt/sbt#2890

@jvican
Copy link
Member

jvican commented Jun 12, 2017

Please, don't merge this. I'd like to review this PR soon and want to think a little bit about this change.

@eed3si9n
Copy link
Member Author

eed3si9n commented Jul 6, 2017

Now that we've agreed on eventual adoption of slash syntax, can I merge this soon?

@jvican
Copy link
Member

jvican commented Jul 6, 2017

I think this is good to go, sorry for blocking. I think enforcing this in the slash syntax is key for its well-functioning.

@@ -13,7 +13,7 @@
{ "name": "type", "type": "String", "default": "Artifact.DefaultType", "since": "0.0.1" },
{ "name": "extension", "type": "String", "default": "Artifact.DefaultExtension", "since": "0.0.1" },
{ "name": "classifier", "type": "Option[String]", "default": "None", "since": "0.0.1" },
{ "name": "configurations", "type": "sbt.librarymanagement.Configuration*", "default": "Vector.empty", "since": "0.0.1" },
{ "name": "configurations", "type": "String*", "default": "Vector.empty", "since": "0.0.1" },
Copy link
Member

Choose a reason for hiding this comment

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

Please can we not make this change? This could be ConfigKey, or similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea. I think it would be good to introduce ConfigRef in this PR.

.callsiteTyper
.context
.enclosingContextChain
.map(_.tree.asInstanceOf[c.Tree])
Copy link
Member

Choose a reason for hiding this comment

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

It's a shame to have this code copy-pasted from sbt's KeyMacro :-/

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you rather have me add util-lhs?

Copy link
Member

Choose a reason for hiding this comment

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

(┛◉Д◉)┛彡┻━┻

😃

@eed3si9n
Copy link
Member Author

eed3si9n commented Jul 8, 2017

Rebased to 1.0 and added ConfigRef.

@@ -0,0 +1,46 @@
/**
* This code is generated using [[http://www.scala-sbt.org/contraband/ sbt-contraband]].
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not in the contraband-scala directory?

package librarymanagement

/** Represents an Ivy configuration. */
final class Configuration private[sbt] (
Copy link
Member Author

Choose a reason for hiding this comment

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

@dwijnand The ctor for Configuration is private.

@eed3si9n eed3si9n mentioned this pull request Jul 9, 2017
@eed3si9n
Copy link
Member Author

eed3si9n commented Jul 9, 2017

Can we merge this so the review surface will be smaller for the API PR?

@dwijnand dwijnand merged commit f8d3718 into sbt:1.0 Jul 10, 2017
@eed3si9n eed3si9n deleted the wip/config branch July 28, 2017 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants