-
Notifications
You must be signed in to change notification settings - Fork 1
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
integrate census3 api client #50
base: main
Are you sure you want to change the base?
Conversation
) | ||
|
||
// Client wraps a client for the Census3 API and other revelant data. | ||
type Client struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh I'm not sure for the need of this wrapper of the census3 client (which is already a wrapper for the API).
The retry mechanism could also be implemented on the original client instead of here.
What's the point @jordipainan ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like to wrap these kind of imports so they can be extended on the consumer, imo adds more flexibility i.e: a new attribute needs to be added.
I dont think the retry mechanism should be added on the base client, and I think thats up to the consumer too. The base client needs to be simplest as possible and the retry mechanism is something one may decide to add or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the methods for the wrapper are just calls to the census3 client with the retry helper. I still don't see the reason to have this wrapper, tbh.
Adding a wrapper adds complexity, and I'm not sure about the flexibility thing. Keep it simple if possible.
Anyway, let's call for more feedback @lucasmenendez
440ac39
to
043e65e
Compare
fb096fb
to
cff04f7
Compare
74645c8
to
c3664a2
Compare
related #36