I'll try to test it manually as much as I understand the changes.

General comment for future: it'd be much easier to review and test if you separated independent changes into individual PRs, instead of cramming everything together.

I found a few potential issues during testing, please let me know what do you think:

1. chartmogul.Invoice.retrieve_by_external_id(config, data_source_uuid='2c6d4a4a-7c1e-11ef-950c-a77409b7d671', external_id='aaa').get() returns AttributeError: type object 'Invoice' has no attribute 'retrieve_by_external_id'. Am I doing something wrong, does it work for you? BTW, can't we extend existing retreive method and allow alternative parameters instead of creating a new method (same for update, etc.)?
2. When I try to disable a Recurly invoice, I still get 404. Does it work for you? BTW, why do we have two separate methods for the same thing - does update_status offer more options, or is it for backwards-compatibility (but I understand from description that this method is just being added)? I would drop this method entirely if possible and accept status field when calling modify instead.
3. chartmogul.SubscriptionEvent.modify(config, data={id:544776074}) produces Please pass 'id' parameter or 'data_source_uuid' and 'external_id'. Does it work for you? BTW shouldn't we pass id as a separate parameter in addition to data, similar to uuid in other objects?
4. Why do we want to allow passing envelope object at all? Shouldn't it always be flat params, like for other objects? I want to be certain, since if we introduce it, it'll be impossible to remove later without breaking backwards compatibility.

@loomchild

• 1, 2, 3: I am so so sorry, I didn't push my most recent changes and have only updated the PR description 🤦.
• 2: status and disabled are different invoice fields, and my understanding is that we aim to have dedicated SDK methods for the public API endpoints, and there are separate Update an Invoice Status and Disable an Invoice

@loomchild also answering your 1st question

BTW, can't we extend existing retreive method and allow alternative parameters instead of creating a new method (same for update, etc.)?

that was my initial approach, until I started actually implementing it: retrieve currently validates that uuid is present, and one would need to change it to "uuid OR (data_source_uuid AND external_id)", which complicates the base Resource validation logic for all resources - it'd be a classic violation of solId's Interface Segregation Principle

I opted on adding separate methods, they make the intent clear.

Thanks for your replies.

2. So now I understand Invoice.update_status is different from disable / enable. What confused me is the example in the description:

result = chartmogul.Invoice.update_status(
    config,
    uuid='inv_e81f78fc-00ab-4a16-95ab-d1f0cd9cd481',
    data={'disabled': True}
).get()

I think valid statuses that can be set are written_off and voided. Please make sure to update the description and check if it works.

4. Ah, so the envelope was accepted before in the SDK. I thought you were just adding it in this PR. Then it makes sense for backward compatibility.

Regarding retrieve, I disagree that it violates SOLID and it makes DX worse by unnecessarily multiplying methods instead of overriding them. Actually, the same criticism can be said about unwrapping convenience logic - you introduced an alternative way of calling the method, instead of adding a new method. We also allow it in other parts of the SDK - e.g. Invoice.all accepts customer_uuid and external_id in addition to uuid. Maybe you need to specialize the retrieve method only for relevant resources instead of modifying it for all of them.

Maybe that's a difference in philosophy of Python's only one way of doing things vs Ruby's expressiveness, but I still think adding a different method for every set of parameters seems over the top. What do other's think?

@loomchild this actually convinces me:

Invoice.all accepts customer_uuid and external_id in addition to uuid

and I am going to extend retrieve's logic, thanks. I'll let you know once it is done.

@loomchild @MariaBraganca I updated moving the params resolution to resource, I am still testing this but it looks good so far and I think you can already take a look - thanks for the thorough reviews

I'll review & testit this afternoon, sorry for delay.

I finally did some more testing, I see that you just fixed an issue in SubscriptionEvents 👍

Here are my comments based on the issues I found previously:

1. Retrieving an invoice by external ID returns a different result type than retrieving an invoice by UUID. It returns a list of entities instead of a single entity, and if unknown parameters are provided, it returns an empty list instead of raising a not found exception. Also, if you specify an empty external_id, it retrieves all invoices from the data source. I think this is incorrect, because there's a separate all method for retrieving entities in bulk.

2. Disable seems to work now. However, when I try to retrieve a disabled invoice, I get a not found error. Is it expected? Is there a parameter that allows me to retrieve disabled invoices?

3. Regarding subscription events, shouldn't we pass the IDs separately from data, similar to how modify works for other entities like Subscriptions, e.g. chartmogul.SubscriptionEvent.modify_with_params(config, id=544776074, data={<properties to update>})? BTW, why can't we merge modify and modify_with_params, what is the difference between them?

@loomchild

1. You are right about that, sorry I thought you were aware it works this way when you suggested single retrieve implementation for both endpoints: The API endpoint GET /invoices?data_source_uuid=...&external_id=... returns {"invoices": [...]} (a list), while GET /invoices/{uuid} returns a single invoice object. Do you think I should unwrap the first result to match the uuid retrieve?
2. Yes there is a query param with_disabled, you need to pass with_disabled=True as a query param: Invoice.retrieve(config, uuid='inv_...', with_disabled=True).
3. Very very good point, thank you, I implemented it this way bcs API requires passing the id to the envelope, I'll update to match other methods

1. Yes, I think you should unwrap it. Or if you think current interface is fine, then don't change retrieve method at all, since fetching invoices by data_source_uuid & external_id already works via the all method and is consistent.
2. Thanks, it works 👍
3. Thanks, it seems to work for modify_with_params method. But why not for retrieve or modify (in a backwards-compatible manner)? Also, why do we need a new modify_with_params method instead of extending modify?

@loomchild

3. it is already there, and the method naming is consistent with other SDKs, I need to keep it like this for the backwards-compatibility

So after you broke it down to separate PRs, we don't have to worry about this one anymore, right?

yes, I decided to break it down into multiple smaller PRs, because it was both harder to work on it and to review it I imagine - closing it