added delivery tojson #462
Conversation
| * @see Visit {@link Actions.Delivery|Delivery} for an example | ||
| */ | ||
| constructor(deliveryKey?: string, deliveryType?: FormatQualifier | string|number) { | ||
| constructor(deliveryKey?: string, deliveryType?: FormatQualifier | string|number, modelProperty?: string) { |
There was a problem hiding this comment.
We should avoid changing the signature of constructors for the model, since the model is internal and users don't care about it
There was a problem hiding this comment.
the api that is changed is not part of the end user's api so its okay to add the model properties here
There was a problem hiding this comment.
I think that having to pass a property name to the parent constructor is a bit hacky and not the simplest path to take. Someone seeing it for the first time (me included) will not immediately understand why it's needed.
How about we do like you've done for DeliveryFormat - update the model in the action's constructor, this means adding action classes if they are missing. this is something you should consider because when we'll want to implement the fromJson, we might need those classes anyway. see here: https://github.com/cloudinary/js-url-gen/blob/master/src/internal/fromJson.ts#L19
There was a problem hiding this comment.
I've went ahead and actually streamlined everything into re-using the DeliveryAction.
We can choose to create a new class for each piece of code, but I'll just remind that this is a temporary solution to this problem, and we must revisit everything in the near future (So we should design with care).
I'm okay with both solutions (Many classes, or reusing deliveryAction), but we should be aligned and avoid half-half solutions.
There was a problem hiding this comment.
I don't like this direction, in addition to what I've already wrote, it also violates the O in SOLID as far as I can tell. But since it's temporary we can end this discussion here.
| * @see Visit {@link Actions.Delivery|Delivery} for an example | ||
| */ | ||
| constructor(deliveryKey?: string, deliveryType?: FormatQualifier | string|number) { | ||
| constructor(deliveryKey?: string, deliveryType?: FormatQualifier | string|number, modelProperty?: string) { |
There was a problem hiding this comment.
I think that having to pass a property name to the parent constructor is a bit hacky and not the simplest path to take. Someone seeing it for the first time (me included) will not immediately understand why it's needed.
How about we do like you've done for DeliveryFormat - update the model in the action's constructor, this means adding action classes if they are missing. this is something you should consider because when we'll want to implement the fromJson, we might need those classes anyway. see here: https://github.com/cloudinary/js-url-gen/blob/master/src/internal/fromJson.ts#L19
|
I've pushed a commit to streamline the use of DeliveryAction - I've moved the logic from Quality and Format internally into Delivery Action, which hopefully adds more clarity to the actionModel. I've also added types for the actionModel: |
Pull request for @cloudinary/url-gen