Reorganizing modules

[hillairet]

The current file/module organization lacks coherence at this time.
It's not clear why session_token.py belongs in "utils" for example.

I think we have a natural organization for the files that would match the documentation menu to highlight the Shopify features handled by SPyLib:

spylib
├── admin_api.py       # Instead of tokens.py
├── hmac.py            # Exposing to the user more directly. Maybe should be added to docs menu.
├── oauth
├── session_token.py
├── webhook.py

This way we have a natural organization for future Shopify features that SPyLib handles and it makes the imports a lot more logical:

from spylib.session_token import SessionToken

Also we should reorganize the webhooks into functions to avoid making the Token class bloated. We can simply have:

from spylib import webhook

webhook_response = webhook.create_http(
    offlinetoken=offlinetoken,
    topics=['orders/create'],
    callback_url=`https://example.com/webhooks
)

[hillairet]

Oh I forgot to say, with this we also can switch from file to folder whenever needed cause both are modules in python and not change for the user.

From

spylib
├── webhook.py

To

spylib
├── webhook
    ├── __init__.py
    ├── create.py
    ├── create_http.graphql
    ...

[hillairet]

Question: Should we have the functions and classes imported in spylib/__init__.py or keep them in their module to make the import more explicit. Using the multipass from #8 as example, it means:

We allow this

from spylib import Multipass

or we enforce this:

from spylib.multipass import Multipass

My preference would be in the latter just to organize the code per feature explicitly.

[lishanl]

I prefer the absolute imports using namespace too

from spylib.multipass import Multipass

is there a way to enforce that? ..

[qw-in]

+1 for

from spylib.multipass import Multipass

[lishanl]

yea it's true that Token class is getting big and maybe deviating from the single responsibility principle with the webhook stuff.

[qw-in]

The webhook stuff seems relevant to Token to me, considering it is just syntactic sugar over execute_gql? At least on the registering webhooks side

[qw-in]

Also we should reorganize the webhooks into functions to avoid making the Token class bloated

Isn't the idea to have one class responsible for calling the admin api to handle rate limit, errors, etc? I don't think splitting it out gains us anything but does make using webhooks a little more annoying

[hillairet]

We are not splitting it. I think Lishan said it best: The Token class shouldn't not responsible for the webhooks initialization. It's responsible for admin API calls so that's what it will do.
Talking about splitting, right now the webhook code is split between a couple of functions on one side and 3 methods in the Token class on the other. So if anything we are not splitting but putting back together what belongs together:

• One class for all admin API calls
• One module for all webhook management

Instead of:

• One class for all admin API calls and webhook initialization
• One module with remaining webhook management stuff

[qw-in]

I guess what I'm stuck on is that webhook initialization is an admin api call. But I'm outvoted so I'll let it go. I can see the idea is to move all webhook related code to one place

In your example is offlinetoken an instance of Token?

[hillairet]

The webhook initialization makes an admin API call, but so does reading an order, creating a product, etc ... do you propose to define all of them in the Token class?

OfflineTokenABC inherits from Token and one has to define their own OfflineToken inheriting from offlineTokenABC so yeah offlinetoken is an instance of Token

[lishanl]

yea, I think the idea is webhook will use the offlinetoken instance (though privatetoken should be eligible too) to do the webhook specific stuff.

[qw-in]

Gotcha - yeah sounds good