search

tarsil / saffier

The only ORM that you will ever need for python.
https://saffier.tarsild.io
MIT License
58 stars 4 forks source link

refactor: check that the user-defined table has 1 and only 1 primary … #127

Open vvanglro opened 8 months ago

vvanglro commented 8 months ago

Close #123 .

vvanglro commented 8 months ago

@tarsil Like this?

tarsil commented 8 months ago

Ok, I think I understand what is happening here. I will have a look later. Thank you for submitting the PR @vvanglro 👍🏼

tarsil commented 8 months ago

@vvanglro although your change is great my concern is for the people already using Saffier as this will break the existing codebase.

The purpose of automatically generate the primary key is for you not to worry about declaring it. Pretty much like Django does and this change will force everyone to add an ID into the existing codebase.

vvanglro commented 8 months ago

My suggestion would be to release this change as a larger release. For example v1.4.0 or v2.0.0

tarsil commented 8 months ago

I need to think about this honestly since this can cause a world of trouble and people can simply stop using it because of it. Very unlikely but it’s a possibility.

Another thing that should be added if we proceed with this step is to add a warning or several in the docs explaining that prior to version 2.0.0 (not v2.0.0), the ID was automatically added by Saffier but from the release 2.0.0 that will be managed by the user providing more control and freedom over the primary keys.

This should probably be included in this PR too.

Something like this if this make sense?

vvanglro commented 8 months ago

Yes, this also fixes the bug that would report multiple primary keys if the user-defined primary key field had a name other than id.

You can add deprecated auto-add primary key field warnings to subsequent 1.x releases to transition to version 2.x. Until you think you can release 2.0.

tarsil commented 8 months ago

Yes, this also fixes the bug that would report multiple primary keys if the user-defined primary key field had a name other than id.

You can add deprecated auto-add primary key field warnings to subsequent 1.x releases to transition to version 2.x. Until you think you can release 2.0.

The deprecation warning should definitely be added but the AutoMixin happens when the ID is an integer.

@vvanglro what I would love to see it tested here since you changed is some tests proving that you can add different primary keys with different names and types (based on what you are trying to solve)?

If that is ok and passing + current tests, then I will proceed with initial pre-releases of Saffier with the warnings before merging this and do the big release.