atomist-attic / rug

DEPRECATED Runtime for Rugs
GNU General Public License v3.0
53 stars 13 forks source link

Parameters cannot be inherited #646

Closed Lawouach closed 7 years ago

Lawouach commented 7 years ago

Assuming you have a rug definining parameters through the decorator. When you inherit from that rug and add new parameters, the ones from the base class do not appear in the subclass rug's definition.

Note that if your subclass doesn't add parameters at all, then the base class ones will show up in that definition.

I think we should have a consistant behavior that parameters can be inherited.

Likely because this function doesn't walk up the chain: https://github.com/atomist/rug/blob/master/src/main/typescript/rug/operations/Decorators.ts#L19

kipz commented 7 years ago

@Lawouach what behaviour do you want here? In the general case get_metadata is behaving well I think. In this specific case, do you want the object and its prototype's parameters to be merged? What if there are conflicts?

Lawouach commented 7 years ago

I think my gut feeling says merging and overriding. Not sure what you mean by conflicts here.

If not, we should find a way not to surprise users as I were. It is a tricky one to track because, though I can inherit from handlers, their parameters aren't passed down as I was expecting. It's only when I saw a log without the base parameters that I realised of that behavior.

That being said, this article led me to that all the way to the init() pattern that we used to workaround the limit I describe here.

Maybe, we could promote such a solution more clearly as well?

kipz commented 7 years ago

By conflicts, I mean that a Parameter with the same name exists in more than one place. Overriding would be a way to resolve these conflicts. It'd be pretty easy to fix this in the @Parameter decorator, by just grabbing the parameter list from the prototype, and merging & overriding with the inheriting object.

Lawouach commented 7 years ago

Maybe, it's just me but the least surprising would indeed be overriding. Maybe this is the wrong assumption though.

kipz commented 7 years ago

Fixed in master.