Feature/token refactor

[Nathan-Nesbitt]

No description provided.

[hillairet]

I know it's a work in progress so probably some of the comments are not applicable. But I thought I'd check things out to guarantee alignment early on.

[Nathan-Nesbitt]

@hillairet This looks like a big PR, most of the changes are just moving files around in testing to support the tests for the two new tokens.

This is what I have at the moment, these were just quick tests to be sure that what I changed works, but I think it could just be one unit test with different parameters for the online and offline tokens:

Let me know your preference for layout!

[hillairet]

Cool looks like we are getting there.
We have to be careful not to retest the same code and double our tests code base.
The graphql and rest methods belong to Token, not OfflineTokenABC or OnlineTokenABC. So there is no point testing the GraphQL and Rest code twice. You can create a class in your test code inheriting from Token and testing Rest and GraphQL directly. We don't need to test python's abstract classes mechanisms.
Testing the initialization is fine though with abstract classes, not sure what we are really testing.
Also let's not make the directory structure more complicated and deep than it needs to be:

/tests
     |- token
            |- test_initialization.py    # for the initialization of OfflineToken and OnlineToken
            |- test_graphql_base.py
            |- test_graphql_operation_name.py
            |- test_graphql_throttling.py
            |- test_rest.py

Also instead of shared.py you should use pytest fixtures which will be much better than having from ...shared import xxx

[Nathan-Nesbitt]

This should be stable now, there is one error in the typing

spylib/token.py:235: error: Argument "headers" to "post" of "AsyncClient" has incompatible type "Dict[str, Optional[str]]"; expected "Union[Headers, Dict[str, str], Dict[bytes, bytes], Sequence[Tuple[str, str]], Sequence[Tuple[bytes, bytes]], None]"

This comes from the AsyncClient.post() method, and doesn't match their docs for what the expected input is.

I have opened a discussion with the devs on httpx as it is not clear what the input should be, but I will update this as soon as I know what's going wrong.

[hillairet]

The README.md needs a good update.

I know I suggested that but now looking at the initialization test, I'm not sure what they are testing. We want to be testing our code, not python itself. You wrote a save token and load token for the test at you are testing that ... not sure what we are verifying here ...
It's ok to have them but they are kind of useless, at least part of the test is.

[hillairet]

Ok on closer second inspection:

• test_token, test_online_token, and test_offline_token are ok as they test some default values
• test_save_token, test_load_token, test_save_offline_token and test_load_offline_token test the testing code you wrote in conftest.py so let's remove them. Let's not maintain code testing the test code. When we have a large set of tests, we don't want to have to keep these ones up to date too.

[hillairet]

More than I expected.
Now that there are changes ... In the docs you put online token before offline token. During the oauth we always get the offline token first since we get the app's token before we get a user's token. I'd follow the same order in the docs.

[Nathan-Nesbitt]

@hillairet this is something I noticed when developing using this using the current design:

Since we aren't defining the __init__ for the classes it is using the default text for a pydantic object. I know the hope was to not have to do this, but can we add in the __init__ as it helps by showing the expected arguments? Unless you know a trick to get this functionality without it?

[hillairet]

@hillairet this is something I noticed when developing using this using the current design:

Since we aren't defining the __init__ for the classes it is using the default text for a pydantic object. I know the hope was to not have to do this, but can we add in the __init__ as it helps by showing the expected arguments? Unless you know a trick to get this functionality without it?

Please make an issue for this and let's do that later. This PR doesn't have to be perfect or complete.

[hillairet]

We might get there eventually.

[hillairet]

🎉 🥳 🎊