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

Add .getId() method #40

Open
jgaehring opened this issue Sep 24, 2021 · 3 comments
Open

Add .getId() method #40

jgaehring opened this issue Sep 24, 2021 · 3 comments

Comments

@jgaehring
Copy link
Member

jgaehring commented Sep 24, 2021

Similar to farmOS/farmOS.py#42.

This is by no means a blocker, since it's not really necessary for anything on the FK Roadmap (farmOS/field-kit#373), and it can always be added at any time down the road w/o being a breaking change, but it's such low hanging fruit and a significant optimization it's worth considering.

@jgaehring jgaehring added this to the 2.0.0-beta milestone Sep 24, 2021
@jgaehring
Copy link
Member Author

I may wait on this a little longer. I guess I'm just reluctant to add another method like this, expanding the API surface.

As an alternative, I wonder if it might make better sense to add an id parameter to the fetch options, which takes a single UUID. I don't know if that is any better, honestly, but it at least feels more aligned with the current API design.

@jgaehring jgaehring mentioned this issue Nov 16, 2021
37 tasks
@paul121
Copy link
Member

paul121 commented Nov 16, 2021

As an alternative, I wonder if it might make better sense to add an id parameter to the fetch options, which takes a single UUID. I don't know if that is any better, honestly, but it at least feels more aligned with the current API design.

I would really recommend against this. The response type is different when you GET a single id as a path argument. From the original farmOS.py issue:

Another reason to move the ID to a separate method is because the response of a single resource is different than the response of multiple resources. Fetching a single resource, such as /api/log/observation/f1790e98-8762-4c16-bb93-ef53f0f77dfe returns a single object in the response data property. Fetching a collection of resources returns an array in the data property.

@jgaehring
Copy link
Member Author

I would really recommend against this. The response type is different when you GET a single id as a path argument

Ohhhh, yes, thank you! I was struggling to remember our conversation around this, and yes, I definitely don't want to do this.

Ok, I think I will punt on this though for now, at least until there is a clear need for such a method. I'd be inclined to implement it in the standalone export of the client, but removing it via adapter, so it is not present on the main farmOS instance.

@jgaehring jgaehring removed this from the 2.0.0-beta milestone Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants