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

Introducing AsyncIO Support & Request Timeouts #18

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

zikphil
Copy link

@zikphil zikphil commented Mar 1, 2023

Good day,

While I know that your python library is automatically generated from a OpenAPI spec, I still had to modify it to work in my asyncio-based project so I decided to contribute it back.

In a nutshell the concept is that your codebase should be async first inside the main repository and that at build time it converts the async files to provide a sync/standard version of the client as well. This is done using unasync. You can test this by cloning the repo, you will notice that there are no knockapi/sync_client folder but if you run python3 setup.py build and have a look inside the build folder, you will see that the sync version of the client has been included in the package. So you have the best of both worlds where you don't need to copy/paste your code twice everytime you make a change and you also offer ur customers a way to use asyncio.

While doing this I also added a read_timeout to fix some of the issues in #13 and modified the CI jobs and tests to be asyncio compatible. The only downside is that we need to drop Python 2 support and require >3.6. Honestly that should not be a big deal since you should not being Python 2 anyway.

Let me know if you have any questions.

Phil

Copy link

@will-afs will-afs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just proposed some minor improvements, but globally, LGTM.

Good job! Having both an async/sync client generated at once is going to be very useful.

return r.json()
except JSONDecodeError:
return None
from knockapi import __version__
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PEP8 convention about modules and packages naming states:

Modules should have short, all-lowercase names. Underscores can be used in the module name if it improves readability. Python packages should also have short, all-lowercase names, although the use of underscores is discouraged.

So files such as Knock.py should be renamed as knock.py (lower case). Same for async_client package which should be renamed as asyncclient (no underscore).



class Knock(Connection):
class Knock(object):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this PR requires "to drop Python 2 support and require >3.6.", there is no reason to make classes inheriting from object

Suggested change
class Knock(object):
class Knock:

Comment on lines +1 to +3
[build-system]
requires = ["setuptools>=62.3.0", "wheel", "unasync"]
build-backend = "setuptools.build_meta"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this thread indicates:

As of pytest-asyncio>=0.17 if you add asyncio_mode = auto to your pyproject.toml or pytest.ini there is no need for the marker, i.e. this behaviour is enabled for async tests automatically.

See https://pytest-asyncio.readthedocs.io/en/latest/reference/configuration.html

Suggested change
[build-system]
requires = ["setuptools>=62.3.0", "wheel", "unasync"]
build-backend = "setuptools.build_meta"
[build-system]
requires = ["setuptools>=62.3.0", "wheel", "unasync"]
build-backend = "setuptools.build_meta"
[tool.pytest.ini_options]
asyncio_mode = "auto"


def test_list():
mocked_client = Mock()
@pytest.mark.asyncio
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As indicated above, @pytest.mark.asyncio occurences can be removed from
tests whenever asyncio_mode = "auto" figures in pyproject.toml

json=payload if method != 'get' else None
)

async with r:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation behind having an async context manager around r here?

@@ -118,7 +118,22 @@ client.users.set_preferences(
client.users.get_preferences(user_id="jhammond")
```

### Signing JWTs
## AsyncIO Usage
The library utilises the [unasync](https://github.com/python-trio/unasync) library to provide full AsyncIO support, you just need to import the AsyncKnock client.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The library utilises the [unasync](https://github.com/python-trio/unasync) library to provide full AsyncIO support, you just need to import the AsyncKnock client.
The [unasync](https://github.com/python-trio/unasync) library is used to generate synchronous code from asynchronous one.
As a consequence, this SDK provides full asynchronous (resp. synchronous) support: to work asynchronously (resp. synchronously), you just need to import the AsyncKnock (resp. Knock) client.

@will-afs
Copy link

@zikphil @cjbell @brentjanderson Until this PR is approved and merged, I think I'll make a fork from it. I had a few questions though:

  • do you know how to access Knock's OpenAPI schema (what's the URL pointing to it?)?
  • is it open to anyone, or just to the Knock Labs organization members?
  • do you know which solution is being used by Knock Labs to generate the Python SDK client from Knock's OpenAPI schema?
  • couldn't this solution generate a sync/async client directly? I think a such solution can for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants