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

Define shopify REST and GQL endpoints as BaseModels #28

Open
Nathan-Nesbitt opened this issue Sep 20, 2021 · 5 comments
Open

Define shopify REST and GQL endpoints as BaseModels #28

Nathan-Nesbitt opened this issue Sep 20, 2021 · 5 comments
Labels
Request: Question Further information is requested Type: Enhancement New feature or request

Comments

@Nathan-Nesbitt
Copy link
Contributor

Since the endpoints are already defined by Shopify, could we type them so we can easily change them?

For example this would be a way of querying the customer rest endpoint:

store.shoprequest(
        goodstatus=200,
        method='get',
        debug='Error',
        endpoint=f'/customers.json?fields=tags&ids={custid}',
    )

Could we instead change this so we have a way of passing in an object with the correct parameters?

endpoint = Customer(field=tags, ids=custid)

store.shoprequest(
  method='get',
  debug='Error',
  endpoint=endpoint
)

This would provide typing and validation, along with the ability to centralize how the REST API is structured allowing easy updates if variables change.

@Nathan-Nesbitt Nathan-Nesbitt added Request: Question Further information is requested Type: Enhancement New feature or request labels Sep 20, 2021
@Nathan-Nesbitt
Copy link
Contributor Author

Same with GQL but not with the endpoint, but rather the query parameter.

https://github.com/SatelCreative/satel-spylib/blob/3bfc32f5eee8d632d85ebd5c54e37e3d9966d4a6/spylib/store.py#L150-L152

@hillairet
Copy link
Member

If we can get the openAPI spec file from Shopify, that's a good idea ... otherwise too complicated to maintain.

@Nathan-Nesbitt
Copy link
Contributor Author

@hillairet could we at least trim some of the data from the request URL? Because I would imagine that the API stays consistent enough to be able to at least provide an interface like this:

store.shoprequest(
  method=GET,
  resource='customers',
  params={
    'fields': 'tags'
    'ids': custid
  }
)

As really the object could just convert back to the original URL format behind the scenes if we don't want to go forward with the API data models.

I think this is easier to understand and harder to screw up than what is going on than the string endpoint param f'/customers.json?fields=tags&ids={custid}'.

@Nathan-Nesbitt
Copy link
Contributor Author

This way, in the future if it is valuable, we could make a set of ENUMS for all of the possible endpoints? (This shouldn't be too bad, as there is not that many endpoints)

store.shoprequest(
  method=GET,
  resource=CUSTOMER,
  params={
    'fields': 'tags'
    'ids': custid
  }
)

@hillairet
Copy link
Member

It's all about the gain, i.e. what problem you solve, vs pain, i.e. keeping it up to date forever.
The gain is really small IMO. You can get a better gain and no pain by checking in the shoprequest if the resource is within the list of known resource and throw a warning if it's not. You get the gain of helping the user debug/avoid mistakes but in a non-restrictive way such that if a new endpoint shows up in the future, the shoprequest will work and the warning can be ignored! The warning could say something like "I don't know this resource but maybe it's a new one I don't know".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Request: Question Further information is requested Type: Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants