Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove *asana.Client function argument from model's methods #3

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

bakurits
Copy link

Before this PR every method of asana's model needed to pass *asana.Client. Now it's embedded inside structs and there is no need for that.

When new models are creating they inherit the embedded client field from the creator.

@kothar
Copy link
Owner

kothar commented Jan 24, 2021

This is really interesting: the embedded client was actually in my original design for this library and removed in bbe591e. My reasoning was as follows:

  • Adding a private field to the objects means that some internal details are not round-tripped during JSON serialisation/deserialisation. This is mostly fine since we know the client is a special case, but adds an additional call to initialise the object before you can use it again. Having the client as an explicit parameter means you can't forget it.
  • If you DO forget to initialise the client in one place, you run the risk of a null pointer panic, or need to check and handle that error everywhere. That's less likely with an explicit parameter where you can consider it a precondition.
  • Embedding the client inside the domain object doesn't match how other API libraries in Go seem to work. But I think I've already strayed away from idiomatic API client design with the API calls being domain object methods instead of client methods.
  • API requests which occur in relation to other domain objects are not possible without initialising the first object. For example, to create a Sub-Task you need a parent Task object, but in this PR there's no way to initialise this without fetching it first to set the client reference. This means that a NewTask(client) constructor is needed if you know the ID and just want to make the CreateSubtask or SetParent API calls.

I'm prepared to be convinced this way is better, but it's a breaking change to the API of the library which will necessitate a new major version to avoid breaking existing code. Do the points above affect your use case, or are you purely using the API client in a fetch-then-modify style which won't require construction of requests from previously stored IDs or serialised records?

Very interested to hear your thoughts on the pros and cons!

Thanks,
Mike

@bakurits
Copy link
Author

bakurits commented Jan 24, 2021

Thanks for the quick response @kothar. Interesting to see your previous commit. I've been using Trello's SDK lately and they have embedded client inside models. see creation of Boards list

I just thought this approach is too verbose, but after seeing your point it seems reasonable.

I think there is a way to solve those problems, by decoupling input and output types of API. So what about instead of Task we return the TaskWrapper type from every function, that has Task it, along with the unexported client field inside it.

I'll think about the design like this and reply here.

Thanks for your time.

kothar pushed a commit that referenced this pull request Sep 30, 2022
feat: tasks.go added custom fields for update task request

Approved-by: Mike Houston
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants