-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 ['override' => true] option in task #1487
Comments
IMO recipes are already rather hard to handle and error prone when customizing. Allowing this level of overrides will make it even more error prone. IMO one should copy and change the recipe instead which also results in less complexity and a more reasonable file for the user |
@staabm Disagree. Recipes really nice approach, over time Deployer recipes with help from dozen contributors got it shape, features and better error handling. This proposal about more explicit way of overriding tasks. Which already happens. |
As alternative solution: Leave API as is and in debug mode
|
I didnt say that the recipes are bad, but only that working with them, because of lots of variables in it is DX wise a real problem. people shouldn't re-use only parts of the recipe. if the need 80% of them, just copy it and adjust what you need helps them in the long run and also keeps the builtin-recipes straigt forward and maintainable. the task of writing a good deployment recipe is already rather hard and gets even more complex the more tools you have. having even more dynamic parts in it doesnt help to make it more reliable. |
I think that if you are using 80% of a recipe, you shouldn't copy it whole. That way you will branch off from the original and won't benefit from the community work that is put into the recipe in the future. For instance, if you want to just disable one task you now need to overwrite it. How about we have some kind of easy wrapper function called And OT, I do think it's a good solution to explicit mark some task to be overwritten and otherwise throw an error. It's not much work to add the Only displaying this in the So my vote is: yes, implement it and throw an error. |
In my opionion you already expressed the problems I also had in #1488 and this is the reason why I think that reusing the recipe and override stuff isnt a good idea. |
@staabm actually this proposal makes overriding harder (because you need to add |
since its already possible I am +1 for making it explicit. |
Description
Now you can override task really simple:
What I'm proposing is to add explicit parameter for overriding:
With out
'override' => true
override will fail with exception:The text was updated successfully, but these errors were encountered: