search

Hadron / carthage

Carthage is an Infrastructure as Code (IAC) framework
Other
7 stars 4 forks source link

Update add_setup_task to new setup_task API #23

Closed kdienes closed 2 years ago

kdienes commented 2 years ago

This updates add_setup_task to work with the changes from 331d165bce2a49ca3a7451b25dfce8f3e65b0f16.

The TaskWrapper interface now requires a description, so we add that as an option to add_setup_task with a boring default.

The stamp property is now generated by overriding in the TaskWrapper subclass and cached as .stamp as as @memoproperty, or set by __set_name__ (replacing the memoproperty).

Here we special-case the kwargs processing of 'stamp'. If 'stamp' is provided as a kwarg, we replace the .stamp memoproperty in the same way as set_name, and set .stamp_is_explicit to True. We use .stamp_is_explicit to ensure we haven't created a confusing situation where both add_setup_task and set_name set the stamp name.

I'm not at all sure if the deconfliction using .stamp_is_explicit is necessary. I'm also not sure if there isn't a better way to set .stamp than passing it in in entirety.

hartmans commented 2 years ago

I don't think you need stamp_is_explicit. My suspicion is that this interface has not been updaded since the time when the argument to setup_task was a stamp not a description. I'll fix this, but with a somewhat different approach. I'm renaming the second argument to description instead of stamp. Stamp will no longer be required, but will be explicitly handled if supplied.

While I'm at it, I think I'll redo the entire API and permit passing in a TaskWrapperBase not just a function. That will change the order of things though.

kdienes commented 2 years ago

Sounds great. I should have marked this as WIP but keep forgetting that's an option.

Stamp will no longer be required, but will be explicitly handled if supplied.

That sounds ideal I just couldn't easily figure out how to do it.