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

Add taskRules flag to NodeExtension #117

Closed
wants to merge 4 commits into from
Closed

Conversation

nhoughto
Copy link
Contributor

No description provided.


this.project.afterEvaluate {
if (this.config.taskRules) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure about this bit?

@@ -33,6 +33,8 @@ class NodeExtension

boolean download = false

boolean taskRules = true
Copy link
Member

Choose a reason for hiding this comment

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

I think we might need a more descriptive name for this, but right now I can't think of a good one

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 went back and forth about it, it is a gradle plugin so in theory shouldn't be abstracting gradle concepts, task rules is the concept being configured. Could be 'dynamic task generation' or something but that is re-labelling something that already has a name.. a task rule 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added some doco to solve for descriptiveness =)

@bsautel
Copy link
Contributor

bsautel commented Oct 2, 2020

Thanks for this PR. We will probably have to port it to the master branch because we did not plan to release new features in the 2.x version.
I am wondering whether the default value should not be false. Indeed, if by default it is true, why is this option configurable?

@nhoughto
Copy link
Contributor Author

nhoughto commented Oct 2, 2020

i guess i could make it disableTaskRules? i was really just following the pattern set by download

The reason you want it configurable is so you can turn it off? If you make it off by default that would change the contract for existing users of the plugin.. would break heaps of stuff?

@bsautel
Copy link
Contributor

bsautel commented Oct 2, 2020

I personally prefer disableTaskRules even if you are right, this is not consistent with download.

That's right, turning this option to false by default would break backward compatibility. But the version 3.0 already broke compatibility and it is still possible to integrate this change before its release. It's the reason why I think that we could afford to make this change.

I am proposing that since if this feature is enable by default, you just have not to use it and that's it. What do you think about that?
As I said, I don't use this feature and I would be ok to deprecate it, it's maybe the reason why I suggest to disable it by default. 😆

@deepy
Copy link
Member

deepy commented Oct 2, 2020

I like the name disableTaskRules better, it's more obvious what happens.

We could deprecate it in 3 and see if anyone complains about it, but it does add to the out-of-the-box experience in that you can just apply the plugin and get going immediately.

//if false (the default), it will enable Gradle Task Rules for `yarn_*`, `npm_*` and `npm_*` task names,
//to support automatic task creation.
//if true, it will not register task rules when the plugin is applied,
//so all rule-generated tasks will need to be configured manually (excluding static `yarn_setup`,`node_setup`,`npm_install`,`yarn_install`)
Copy link
Member

Choose a reason for hiding this comment

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

the static tasks don't use snake_case, they're all camelCase. e.g. yarnInstall, npmInstall, nodeSetup

@deepy
Copy link
Member

deepy commented Oct 5, 2020

This looks good to me, I'll test it tonight. I think we can do an additional 2.x release but with 3.x just around the corner that's the main focus.

I've been busy moving to a larger apartment and I've finally got enough room for the computer and gotten enough boxes away so that I can actually reach it. There's some proxy changes I've gotta finish first, but after that I can have a look at porting this to 3 the other PR and merging both so we can get the final 3.x RC online

@deepy
Copy link
Member

deepy commented Oct 14, 2020

With this I can't configure tasks that are created from rules, e.g. the following does not work:
tasks.named('npm_help').configure { doLast { println "hello" } }

So we can't merge this one into 2.x as it breaks backwards compatibility, we should still be able to get it into 3.x however.

@deepy deepy closed this Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants