-
-
Notifications
You must be signed in to change notification settings - Fork 120
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
Use Gradle properties #93
Conversation
…l variant, we now provide some methods that are able to compute the former variant fields. We are going to use Gradle properties to fully work in the lazy mode. The concept of variant that is computed after project evaluation is not really compatible with this approach.
…ation of all tasks to follow Gradle new guidelines.
…cOverrides and nodeModulesOutputFilter.
I realized that I forgot to replace usages of |
…ontain null values. If they are not defined, they are empty and thus contain nothing and not null.
…es that I don't know well and I am not sure I can migrate them correctly to Kotlin.
…. Add a property to run them using all the Gradle supported versions. The CI still run tests using all Gradle versions.
…o be no longer used (artifactory publication). Simply use the Gradle plugin publish plugin to publish it to Gradle portal.
… model accessor for the node extension.
Change the semver configuration to change to the next major version.
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 got two things left to check, but I gotta take a break
val download = project.objects.property<Boolean>().convention(false) | ||
|
||
init { | ||
distBaseUrl.set("https://nodejs.org/dist") |
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.
Does this interfere with distBaseUrl = null
, the way we disabled automatic adding of repositories (for those with corporate environments)?
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, I has to do with unsetting this property.
I learned a few important details regarding the Property
API:
- By default, a property is not defined (its value is null). If you want to unset it, you have to set null value.
- Using a convention is not the same thing as setting an initial value. The convention value is a value that will be used if the value is not defined when retrieving it. That means that if you set a value and unset it, it will return the convention value.
- When the property is not defined (its value is null), calling
.map()
will do nothing. It took me a lot of time to understand that one test was broken because of that. It was about an@Output
annotation returning aProvider
(it sounds like it is recommended to do that). I wanted to map an optional configuration value and define the output according to is value. When the property was not set, theProvider
was considered as empty and the mapping function that handled thenull
case was not called. I had to break the provider chain at this place. I did not find any alternative solution.
To come back to your question, if I use a convention, the property not set but you just have a default value. Unsetting the value won't change anything (since it is already not set). To get it unsettable, we cannot use convention but we have to initially set the value: this will enable the value to be unsettable later.
Thanks @deepy for your feedback! Sorry, it will probably take you some time to review that but getting your feedback is very interesting to build a better product. I selected the issues that we should fix before the 3.x release. After, we will no longer have the opportunity to break backward compatibility. All the issues are in the 3.0 milestone. If you think about other issues that should be fixed before the release, don't hesitate to add them to this milestone. Otherwise, I think that we are close to the release of this new major version! |
As discussed in #39, I rewrote the
NodeExtension
and all tasks to use Gradle properties and providers.The code is a little less readable than before but it works. The Kotlin DSL usage is also a little less beautiful than before because we need to use
.set()
for all properties.I had to remove the concept of variant that was not really compliant with the concept of lazyness since it was computed after project evaluation. It remains a few usages of
project.afterEvaluate()
that I was not able to remove.I also found a better way to handle
execOverrides
andnodeModulesOutputFilter
. They now use GradleAction
s as recommended by Gradle because its neutral and the Groovy and Kotlin DSL are able to map Groovy closures and Kotlin lambdas to Actions.What do you think about these changes @deepy and @jfbibeau?