Skip to content
This repository has been archived by the owner on Jul 13, 2022. It is now read-only.

V2 #20

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

V2 #20

wants to merge 33 commits into from

Conversation

jdrake
Copy link
Contributor

@jdrake jdrake commented Apr 5, 2022

@ssantana-ns @bgimby-ns

This is a proposal for a pynocular v2 implementation. I assume that we'll keep this branch open for a while as we test out v2 before merging. We can still release new versions to pypi.

There are a few goals for this proposed v2:

  1. Provide better model attribute typing support by using regular inheritance instead of the decorator. The PR Better static analysis support #19 uses the same approach.
  2. Refactor the model implementation to allow for multiple backends. This gives the ability to add an SF API integration later. We will need to use the SF UDD to store entities instead of Postgres in production. The hope is that we can keep using pynocular and abstract the storage backend. A nice side benefit of this approach is that we can treat the in-memory implementation as a first-class backend and not have the patch_model decorator need to define a bunch of private functions.
  3. Use the databases package instead of our layer on top of aiopg. This package has a simple interface on top of asyncpg that should work for us. I'm not sure if there are things about asyncpg that don't work for us.

Check out the test module for how it works. The basic usage goes as follows:

  1. Do what you need to do to set up the backend (in the case of SQL it's establish a connection)
  2. Instantiate the backend, providing it with backend-specific arguments
  3. Set the active backend in the aio context using the set_backend context manager. Note that in a real FastAPI service, this stuff will happen once in a middleware function so that application code can just use the DatabaseModel classes as they do now.
  4. Call DatabaseModel methods
async with Database("postgres://localhost:5432/postgres") as db:  # (1)
    with set_backend(SQLDatabaseModelBackend(db)):  # (2) and (3)
        await Thing.select()  # (4)

Notes:

Things left out:

  • Nested model support, since it's not used and adds complexity. I think it should be ok to add it back in but we'd have to be careful about breaking the model/backend abstraction. I vote that we leave it out unless we really need it.

@jdrake jdrake requested a review from ssantana-ns as a code owner April 5, 2022 19:13
@jdrake jdrake requested a review from bgimby-ns April 7, 2022 21:49
Copy link
Collaborator

@ssantana-ns ssantana-ns left a comment

Choose a reason for hiding this comment

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

some questions but high level LGTM

pynocular/backends/context.py Show resolved Hide resolved
pynocular/backends/base.py Show resolved Hide resolved
pynocular/backends/memory.py Show resolved Hide resolved
pynocular/backends/base.py Show resolved Hide resolved
tests/functional/test_model.py Outdated Show resolved Hide resolved
tests/functional/test_model.py Outdated Show resolved Hide resolved
pynocular/model.py Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
pynocular/backends/memory.py Show resolved Hide resolved
pynocular/backends/sql.py Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants