-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add basic structure for API client class #163
Comments
Okay, after writing that out, I may be leaning toward option 2. Do you have any thoughts on this, @niconoe? |
Also @synrg, since you expressed interest in this, do you have a preference between these two options? |
@JWCook: thanks! While option 1 is also perfectIy fine, I really like option 2. It's like a brand new and exciting API (between client code and Pyinaturalist). In that case, I'd also suggest communicate to users in terms that keep the separation very clear and also give guidance: "hey, we have a new major version of Pyinaturalist, you can continue using the old (mostly unchanged API) but there's a new one that's probably nicer and recommended for new projects, here is the tutorials/cheat sheet. I'm not too concerned about the "mixes data with logic" part, in think it's inevitable to some extend, especially for a data-oriented project like this one. But maybe I'm not understanding the issue fully, do you have a specific example that bothers you and that you want to discuss? Just my 2 cents, I think in the end you're in the best position to make the final call! Also curious to hear @synrg, it's amazing to have more people contributing (even if just suggestions, we're better together!) |
I'm also in favour of option 2. Pragmatism wins out here for me over strict separation of code & logic. And if not 2, then something else other than 1, which is not very appealing for the reasons already stated. Regarding the example given, why |
Thanks for the input! My example was for adding that to the current Adding separate classes for model collections is a possibility. I've actually experimented with that a bit over here, but I wasn't quite happy with how that was turning out. It seemed a bit confusing to work with a |
I'm all for keeping things simpler. It wasn't something so much that I thought was wrong as something I didn't have a clear idea yet of how it ought to work. Looking for examples from other API clients as a guide would be where I'd start, too. |
Option 3: API client classAnother slightly different option to consider would be a single API client class containing wrappers for all API functions. There are some good examples of that out there, like Usage could look something like: client = InatClient() # Probably needs a better name
observations = client.get_observations(user_id='my_username') Pros:
Cons:
I need to think through that one a bit more. Any thoughts on that? |
That's exactly the choice I made in Dronefly. I wouldn't hold up my newbie python code as a shining example to follow, though. :) And yeah, it's ... messy with all those methods and no grouping together per resource type, I agree. |
A hybrid approach? Client object for all the settings and stuff, but with each of the models represented thusly (from Gitlab link you gave):
We could have |
Yeah, I like the look of that! |
I'm not sure I fully understand the last example, but the settings discussion made me think of the requests approach which I quite like: you can either call methods (such as |
As for how I understood the last example, This resolves my earlier discomfort with I guess it's kind of like requests, only different, because we have all of these different kinds of resources to contend with, unlike requests, which is much simpler at the top level. So we share more in common with Gitlab. Unsurprising, since both are APIs for websites with numerous interrelated resources on them. |
Exactly, I see that as a good way to get the benefits of both options 2 and 3 without most of the drawbacks we mentioned. So The only downside is, well, it will be more work, but I'm up for it. I should have time within the next week or so to get a start on this. |
Based on a suggestion from Ben, it would be useful to have some default request params that could be set on the client and then passed to any applicable API requests. Possible params include:
|
Continuing this in #217 |
The data models (#145) are mostly complete. The next step is to add support for API requests that return model objects instead of JSON. For backwards-compatibility, this should be an optional feature, not a replacement for the existing API functions.
Update: Decided to go with a compromise between options 2 and 3, described in comments below. This will involve an API client class and one controller class per resource type.
Options
There are at least two ways to go about this:
Option 1: Add a keyword arg to existing functions to specify return format
Example:
Pros:
format
could be used to specify any other supported format, for exampleformat='csv'
.Cons:
Union[x, y, z]
isn't very useful as a return type annotationOption 2: Add wrapper @classmethods to model objects
Example:
Pros:
Cons:
forge
to reuse existing function signatures & docstringsExamples in other API clients:
boto3
, and other service-specific interfaces like S3.BucketThe text was updated successfully, but these errors were encountered: