moigagoo / norm

A Nim ORM for SQLite and Postgres
https://norm.nim.town
MIT License
378 stars 34 forks source link

Feature/#133 add public from row #164

Closed PhilippMDoerner closed 1 year ago

PhilippMDoerner commented 1 year ago

This proc adds the feature requested by #133 : A way to interact with the database with arbitrary SQL, yet still benefit from norms incredibly useful parsing of Row to objects, just this time it allows for arbitrary object types instead of using models.

This has the following benefits:

The specific procs that allows this are:

A small side-benefit: I observed around a 10% speed-up for my limited test-cases when executing the "new" version of fromRowPos. The reason for this speed-up is that some computation has been moved to compile-time instead of runtime.

Things that would still need to be done:

PhilippMDoerner commented 1 year ago

@moigagoo before I do the (not that inconsiderable amount of) work of getting this PR into a "finished" state, I would like your confirmation first that the feature is actually wanted. I'd like to avoid a scenario where I bring this to the finish line, only for us to reject the PR.

PhilippMDoerner commented 1 year ago

Since the actual conversion of Row -> ref object is implicitly covered by all the tests that convered from Row -> Model I personally think that no further tests are required. Just that they generally can work, which the trawselect.nim test files cover.

To me the work on this PR is thus done and it could be merged.

PhilippMDoerner commented 1 year ago

@moigagoo Yo, it's been near a month since creation of the PR with request for comment and almost a week since it finished (which I've done to have a modified branch of norm that I can now use in my sideproject).

Is this to be on ice until the escape column name PR is merged or is something else going on?

moigagoo commented 1 year ago

@PhilippMDoerner sorry for the slow responses. I've looked through the OR and found just one place where I have concerns. Please review this comment and we'll merge the PR.

PhilippMDoerner commented 1 year ago

@moigagoo I've found a new reason for concern that is related to your comment but in a different manner. It turns out there are 2 pragmas that we would be unable to support with this: Using T() makes it essentially impossible to use the requiresInit pragma with a model/ref object. noInit seems similarly affected though I did not test this one. To me that is an acceptable sacrifice, but I thought I should bring it up since it came up in convo with Yardanico.

moigagoo commented 1 year ago

I'll release 2.5.2 Monday. I'm afk for the entire weekend.