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

Add support for writing from dataclasses. #12

Closed
xlevus opened this issue Jan 30, 2020 · 6 comments
Closed

Add support for writing from dataclasses. #12

xlevus opened this issue Jan 30, 2020 · 6 comments

Comments

@xlevus
Copy link
Contributor

xlevus commented Jan 30, 2020

Currently, there is support for reading database rows into dataclasses which significantly reduces the amount of boilerplate required. But the inverse is not the case.

e.g.

@dataclass
class Article:
  title: str
  body: str
  created: datetime

  def create(self, conn):
    return queries.insert_article(conn, title=self.title, body=self.body, created=self.created)
  
  def update(self, conn):
    return queries.update_article(conn, title=self.title, body=self.body, created=self.created)

If the parameters supported dot notation, this could be simplified to

-- name: update_article
UPDATE article SET title=:article.title body=:article.body ...
def update(self, conn):
  return queries.update_article(conn, article=self)
@frederikaalund
Copy link

I'd like to see built-in support for this feature as well.

Temporary work-around:

queries.update_article(conn, **dataclasses.asdict(article))

@nackjicholson
Copy link
Owner

nackjicholson commented Jul 8, 2020

The record_classes feature is becoming my biggest regret with this lib. When I first did it I thought it was really cool because I like using dataclasses too. But now I feel like it's opened up a slippery slope to wanting this library to be more ORM-like.

I don't want to break this library for you all, but I do want to understand your use of it more. I would also like to understand if aiosql were a little lighter weight and didn't do this marshalling to custom record_classes would that actually allow development of a separate and better tool that could be used in coordination with aiosql. I'm seeking to shrink the reach of aiosql and keep it focused on loading SQL from files and providing a basic way to execute those queries from python. Returning dataclasses and accepting dataclasses as input seems a bit beyond that.

Also, I'm opposed to this because as it stands record_classes do not have to be dataclasses. They can be literally anything. We can't just alter aiosql to accept dataclass instances, we would have to alter in a way that users can define their own input marshalling for whatever type of record_classes they use. It becomes a can of worms and I just don't want to do it, it doesn't seem like something to add to this library.

Sorry for being that maintainer 😄 I swear I have the best intentions here. I am getting pretty close to wanting to make a major version bump that removes this API though.

@xlevus

@frederikaalund
Copy link

Thanks for taking a look at this issue.

But now I feel like it's opened up a slippery slope to wanting this library to be more ORM-like.

That's a good point. One of the reasons that I prefer aiosql is that it does not enforce ORM on you.

@xlevus' update and create functions go in the ORM direction. To each his own but I don't want this feature for ORM purposes. I simply want the record_class feature supported in both ends of aiosql: DB output and DB input.

Currently, record_class are only used for DB output. I want to use it for DB input. That's why I like @xlevus' proposal:

-- name: update_article
UPDATE article SET title=:article.title body=:article.body ...

That's it.

I'm seeking to shrink the reach of aiosql and keep it focused on loading SQL from files and providing a basic way to execute those queries from python. Returning dataclasses and accepting dataclasses as input seems a bit beyond that.

I can agree with that. A narrow scope means that the essential features can get full focus. Move all the nice-to-haves to other packages.

I can see three ways forward:

  1. Keep record_class as is (for DB output only)
  2. Deprecate record_class entirely
  3. Add DB input functionality to record_class

My personal votes go to options 2 and 3. Either option will do for me.

Sorry for being that maintainer 😄 I swear I have the best intentions here. I am getting pretty close to wanting to make a major version bump that removes this API though.

You're doing good. :) Saying "no" is important!

@xlevus
Copy link
Contributor Author

xlevus commented Jul 8, 2020

My suggestion goes beyond being an ORM, and my example is probably a dumbed-down example.

I see it as a way of simplifying code. currently, if you have a 'complex' nested object that goes into the database, you need to write some code to flatten it.

Which you can't attach to your aiosql object, because it's an instance so you can't subclass it.
You can stick it next to your aiosql object, but then you're calling queries.flatten_and_call(complex_object={...}) some places, and queries.queries.call(foo=1, bar=2, baz=3) other places. This is kinda gross. I guess you could define a 'proxy' method for every query, but again, gross.

So maybe, I don't think I was necessarily wanting an ORM, but more a path for handling more complex cases, without building something 'over there'.

@nackjicholson
Copy link
Owner

@xlevus Thank you very much for expanding on the ask! Super helpful.

@zx80
Copy link
Collaborator

zx80 commented Jun 25, 2022

I agree that it should not be done.

@zx80 zx80 closed this as completed Jun 25, 2022
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

No branches or pull requests

4 participants