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

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

Closed
octionut opened this issue Apr 26, 2024 · 4 comments
Closed
Assignees
Labels
bug marked_for_imminent_closure Fully or partially obsolete issue. Request clarification, close if none provided.

Comments

@octionut
Copy link

octionut commented Apr 26, 2024

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

  • Initialize a python/poetry project with neomodel = "5.2.1"
  • Run a file containing the import statement above
  • Lock the major version only (e.g. neomodel = "^5.2.1")
  • Update the lock file using poetry update or equivalent
  • The lockfile now specifies neomodel = 5.3.0 should be installed
  • Run the same file that previously didn't fail

Simple Example

Screenshots (where it's possibile)

Specifications (Mandatory)

Currently used versions

Versions

  • OS: MacOS 14.1.1 but it's irrelevant
  • Library: 5.2.1 - 5.3.0
  • Neo4j: 4
@mariusconjeaud
Copy link
Collaborator

mariusconjeaud commented Apr 26, 2024

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 mariusconjeaud self-assigned this Apr 26, 2024
@mariusconjeaud mariusconjeaud changed the title Database was removed from neomodel.util Database was removed from neomodel.util and moved into neomodel.sync_.core Apr 26, 2024
@mariusconjeaud
Copy link
Collaborator

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

@octionut
Copy link
Author

octionut commented Apr 26, 2024

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

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
Copy link
Collaborator

mariusconjeaud commented Apr 26, 2024

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.

@mariusconjeaud mariusconjeaud added the marked_for_imminent_closure Fully or partially obsolete issue. Request clarification, close if none provided. label Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug marked_for_imminent_closure Fully or partially obsolete issue. Request clarification, close if none provided.
Projects
None yet
Development

No branches or pull requests

2 participants