moigagoo / norm

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

Add compatibility for nim 2.0 #182

Closed PhilippMDoerner closed 1 year ago

PhilippMDoerner commented 1 year ago

I know you're already aware of this, but I believe making it an issue would still be useful to document our trail of thoughts as well as progress.

With nim 2.0 the packages std/db_sqlite/db_postgres/db_mysql are no longer part of the standard library!

They have been moved into its own package called dbconnector. I do not believe this affects norm directly as I'm not aware of any std/db_* imports there, but it does affect norm's ndb dependency. It makes use of std/sqlite3 and stb/db_common.

Over there it should be as simple as swapping out the imports:

std/sqlite3 => db_connector/sqlite3
std/db_common => db_connector/db_common

Once this is fixed, it will require an update of the ndb dependency.

Now the question is what to do about this? ndb appears to not be receiving any further work. Should we fork? Or PR to the original ndb upstream? Or remove the ndb dependency entirely and move that code into norm?

The maintainer of ndb does appear to still accept PRs, so working with that repo seems still possible. I personally would prefer not to move ndb code into norm, as the value of a postgres/sqlite wrapper that allows for more correctness is something not just norm can make use of.

moigagoo commented 1 year ago

Now the question is what to do about this? ndb appears to not be receiving any further work. Should we fork? Or PR to the original ndb upstream? Or remove the ndb dependency entirely and move that code into norm?

I think we should send a PR to the ndb repo. If the owner won't respond or accept the PR then we fork.

I'm scared of the idea of just copying ndb's source code into Norm.

PhilippMDoerner commented 1 year ago

Do you manage to get the postgres tests running? The changes itself are trivial, I just can't guarantee the tests run and I don't see pipelines set up on ndb.

I can make the PR in 5 minutes and I can guarantee the sqlite tests run, just not the postgres tests.

PhilippMDoerner commented 1 year ago

Now the question is what to do about this? ndb appears to not be receiving any further work. Should we fork? Or PR to the original ndb upstream? Or remove the ndb dependency entirely and move that code into norm?

I think we should send a PR to the ndb repo. If the owner won't respond or accept the PR then we fork.

I'm scared of the idea of just copying ndb's source code into Norm.

I've made a PR for now, with a big disclaimer that I can't guarantee that those changes work with postgres. https://github.com/xzfc/ndb.nim/pull/22

Should we talk about a timeline at which point we think about a fork?

PhilippMDoerner commented 1 year ago

Update, new PR here: https://github.com/xzfc/ndb.nim/pull/23

It didn't only require updating the imports, you also had to fix some Effects annotations, so TimeEffect had to be attached to multiple places which comes from the std/times lib. Other than that, this branch of my ndb fork allows me to fully compile norm docs, so should be functional:

https://github.com/PhilippMDoerner/ndb.nim/tree/nim2

moigagoo commented 1 year ago

Should we talk about a timeline at which point we think about a fork?

I think enough time has past already :-)

PhilippMDoerner commented 1 year ago

The as of now rebranded lowdb finally has its CI up and running, tests running through and is in the process of getting published :partying_face:

Norm update: soon :soon:

PhilippMDoerner commented 1 year ago

Another important breaking change to keep in mind that popped up in the last 2 months:

Generic type declarations done like this [_] are no longer supported. No idea of official docs about it, but it breaks compilation with this error:

Error: undeclared identifier: '_'
candidates (edit distance, scope distance); see '--spellSuggest': 
 (1, 4): '$'
 (1, 4): '&'
 (1, 4): '*'
... tons of more suggestions...

The reason for that in norm are the various [_] generics as defined in places like here: https://github.com/moigagoo/norm/blob/4479240e6a3347fa8990839b7240d346c4b686a6/src/norm/private/sqlite/dbtypes.nim#L49

The solution is pretty straightforward, replace [_] everywhere with [T] (or similar where T is already used).

We can of course wait if nim decides that this shouldn't be a breaking change, or we can be proactive. I don't really care too much. @moigagoo do you have any strong feelings one way or the other?

moigagoo commented 1 year ago

No strong feelings here. [T] is more readable anyway.