Add set_partial_override method to the API

[magdakwiecien]

Brief Summary of Changes

• Add update_partial_overrides method to the API. This will allow to override gen_fill derived asset result.

What Does This PR Address?

• GitHub issue (Add reference - #XX)
• Refactoring
• New feature
• Bug fix
• Adds more tests

Are Tests Included?

• Yes
• No

Reviewer, Please Note:

[magdakwiecien]

There's a general error in the pipeline run:

> [email protected] dtslint /home/travis/build/cloudinary/cloudinary_npm
> tools/scripts/ditslint.sh
Error: Errors in typescript@local for external dependencies:
../node_modules/@types/markdown-it/lib/index.d.ts(151,33): error TS2694: Namespace 'LinkifyIt' has no exported member 'LinkifyIt'.
    at /home/travis/build/cloudinary/cloudinary_npm/node_modules/dtslint/bin/index.js:190:19
    at Generator.next (<anonymous>)
    at fulfilled (/home/travis/build/cloudinary/cloudinary_npm/node_modules/dtslint/bin/index.js:5:58)

[mckomo-cl]

I would change name to upsert as it does exactly that create or update. It's a unique operation for our API so there is no convention yet for similar operations.

[const-cloudinary]

upsert term comes from the database world and not from API.
See this discussion in stack overflow:
https://stackoverflow.com/questions/18470588/in-rest-is-post-or-put-best-suited-for-upsert-operation

[mckomo-cl]

You are correct, upsert is not an HTTP API-connotated word but for the sake of good DevEx using that name is better than update where there is no create method.

That being said, it's just my suggestion.

[magdakwiecien]

From my understanding of the whole partial overrides thingy, this method will update a resource with partial overrides (and this might be operation that creates an override, or overrides an existing override).

What do you think about the set_partial_overrides name? It's more human-like language than upsert 😉 , and it still might express the nature of this operation better than update.

[mckomo-cl]

I just noticed another thing. The final method name should indicate a singular partial override, as you send a payload of only one partial override.

set_partial_override also sounds good to me

[magdakwiecien]

as you send a payload of only one partial override

Oh, I thought there can be multiple overrides provided.
If that's not the case, why overrides in API payload is an array?

{
        transformation_prefix: string;
        asset_override_uri: string;
        overrides: Array<{
            action: 'gen_fill';
            params: {
                seed: string;
                prompt: string;
                ignore_foreground: boolean;
            }
        }>;
    }

[mckomo-cl]

In the future, you will be able to provide overrides to many actions, not just gen_fill.

E.g. consider w_100/e_isolate,b_gen_fill,.... The correct payload for it would be:

{
   "transformation_prefix":"w_100/e_isolate,b_gen_fill,...",
   "asset_override_uri":"http://...",
   "overrides":[
      {
         "action":"gen_fill",
         "params":{
            "seed":"...",
            "prompt":"..."
         }
      },
      {
         "action":"isolate",
         "params":{
            "seed":"..."
         }
      }
   ]
}

You override one transformation prefix with one override snapshot, but the snapshot is result of override of two actions (gen_fill and e_isolate).

[const-cloudinary]

CC @mikeys ^

[cloudinary-pkoniu]

LGTM.
As long as @mckomo-cl and @const-cloudinary are ok with the naming, I don't have an opinion there.

[jackieros]

LGTM. As long as @mckomo-cl and @const-cloudinary are ok with the naming, I don't have an opinion there.

@mckomo-cl - As per our discussions on the Confluence spec, I wasn't entirely convinced that "partial override" is the ideal name for the feature, but I won't have time to deep dive into it.

I thought it was still in the spec stage. Didn't realize it was already being implemented in an SDK. Have you & @konforti sync'd with @carlevison on the naming of all the methods?