neo4j-contrib / neomodel

An Object Graph Mapper (OGM) for the Neo4j graph database.
https://neomodel.readthedocs.io
MIT License
936 stars 231 forks source link

`Database` was removed from `neomodel.util` and moved into `neomodel.sync_.core` #793

Closed octionut closed 3 months ago

octionut commented 4 months ago

Expected Behavior (Mandatory)

The following python command, which worked in 5.2.1, should also work in 5.3.0:

>>> from neomodel.util import Database

There should not be any breaking changes for minor version updates. As we use typed python, this has rendered our applications to fail running after a poetry update!

Also this breaking change is not specified in your changelog/readme, we had to check your source code to find out what was happening.

Actual Behavior (Mandatory)

>>> from neomodel.util import Database

E   ImportError: cannot import name 'Database' from 'neomodel.util'

How to Reproduce the Problem

Simple Example

Screenshots (where it's possibile)

Specifications (Mandatory)

Currently used versions

Versions

mariusconjeaud commented 4 months ago

Hello ! So.... That is a problem with the current versioning scheme of neomodel. The choice was made originally to make the versioning not fully semantic, but rather : Neo4jVersion.major.minor

So technically, 5.3.0 IS a major release according to this scheme. Sorry if you were not aware and it broke your application ; I thought long and hard about moving to a real semantic versioning now but in the end did not. Now it makes me doubt my choice again 😓 . I did write in the README that 5.3.0 would be a breaking change-introducing release some months ago though - but I know this is a limited option.

As for the Database class, with the work around async, it had to be moved into here : neomodel.sync_.core. That is because neomodel.util is now async-independent. Note that this means there is also a new AsyncDatabase class, located in neomodel.async_.core I initially did not mention this in the Changelog & README, because Database is considered an "internal" class, since neomodel provides the db/adb singletons for you. So I updated Changelog and README

mariusconjeaud commented 4 months ago

(Note : I took the liberty to update your issue's title, so that people can see the fix straight away)

octionut commented 4 months ago

Thanks for the quick reply and for the suggested fix; we're fine now but it was quite a surprise this morning! :grin:

So technically, 5.3.0 IS a major release according to this scheme.

I slightly disagree with this approach though, because of the way most package managers handle version pinning and how most people are used to pin versions in their requirements files. I would suggest either not introducing breaking changes for SemVer minor versions, or specify this in the documentation, so people can use tilde requirements for neomodel only (if there is already a line there, I am sorry, I completely missed it).

For now we'll just pin this to ~5.3.0 in order to avoid having issues when updating to 5.4.x.

Thanks!

(This is also a quirk of python's lack of private)

mariusconjeaud commented 4 months ago

Totally agree with you ; I have gone through the documentation again, trying to find where the versioning scheme was explained and... You're right, it's not in there. So that is definitely something we have to add.

Maybe we will go away from pinning Neo4j version as the first digit in the future ; it's a tough call, because also the scheme today shows you right away which version of Neo4j is supported. Let's see !

I will leave your Issue open for a few days still, so any people coming can see this if they're wondering.