Closed W1ldPo1nter closed 5 years ago
First off I'd like to thank you and your colleagues for the contributions on behalf of everyone using this library! Looking forward to more PR's coming in! 😄
Yep, a few of these clients are particularly gross name-wise at the moment. I'm still thinking about which naming convention I like the best, but I'm open to suggestions as well!
I think at the moment, I prefer the more explicit names (perhaps like update_by_email
and update_by_id
), and deprecating the regular update
method (and also the ones ending in the a_contact
because I agree that it's not great that way!). It's also a bit odd here because in a few other clients, you can only lookup by ID, and so we just use the basic get
delete
and update
method names like in the CompaniesClient
: https://github.com/jpetrucciani/hubspot3/blob/6791c78e8ec722a012615ef3d9badc33fb68954f/hubspot3/companies.py#L34-L47
I think that inside ContactsClient
they should be renamed from update_a_contact
and delete_a_contact
to update_by_id
and delete_by_id
respectively (and deprecating the former) to make a bit more sense, since we're already in the context of Contacts, so having the word contact in the method name seems a bit redundant! And also doing the same to get_contact_by_email
and get_contact_by_id
-> get_by_email
and get_by_id
may make more sense in the long run.
Also to the point of deprecation notice, we could just say next major version bump, or just sometime in the future! I do know it may be annoying to have something small like that break, but it'd be a pretty quick fix whenever someone upgrades the library - we'd just want to make sure that we have it documented in the release notes!
Thanks for the quick reply!
We are also preferring concise names without redundancy, so we'll make these changes the way you proposed. A PR will come your way as soon as we got this done (including the new endpoints).
I'm closing this since these questions were specific to the Contacts API and the corresponding changes are already pulled. Thanks for the quick answers and pulls.
My colleagues and I are currently in the process of extending existing clients by adding more endpoints (mainly contacts) and adding new clients (e.g. Ecommerce Bridge) - PRs will follow. ;-)
When it comes to the Contacts API, one of the endpoints we want to add is the Update a contact by email endpoint (currently,
hubspot3
only offers the corresponding endpoint that works with IDs). To implement this as a new method, we will have to choose a name for it - which is why we found that the methods of theContactsClient
use multiple different naming conventions:by_id
/by_email
suffix, e.g.get_contact_by_id
a_contact
suffix, e.g.update_a_contact
update
The first two conventions seem to follow the names from the endpoint overview on the left side of the HubSpot API docs, although the
a_contact
suffix is not necessarily nice.The
update_a_contact
andupdate
methods also seem to contain the same logic and are therefore duplicates, which means that one of them should probably be removed in the long run.My questions are:
update_a_contact_by_email
to follow the names in the HubSpot API overview or should it be calledupdate_contact_by_email
to follow the convention of theget_...
-methods (while renaming the currentupdate
method toupdate_contact_by_id
)? Or even something shorter likeupdate_by_email
?update
method and other deprecations? I would generally suggest to change one of the duplicate methods to give a deprecation warning and then simply call the other method (that way, the duplicate code is removed, but both methods remain for implementations that already use them; the method containing the deprecation warning can then be removed in the future after developers had the time to react to the warning). The same could be applied to methods that would potentially need renaming after choosing a naming scheme in the first question. Is there a deprecation policy so we can already include the version number in which such methods would finally be removed in such warnings (we could also just say "will be removed in the future")?We would implement all of this when extending the
ContactsClient
and throw a PR your way - we just need to know what the preferred naming convention/deprecation policy would be.