Default model pk: id = fields.IntField(pk=True) #34
Conversation
Pull Request Test Coverage Report for Build 88
💛 - Coveralls |
Pull Request Test Coverage Report for Build 77
💛 - Coveralls |
If a pk isn't defined, we create a pk called id for that model. Also added Model method tests.
grigi
force-pushed
the
feature/model_tests
branch
from
e472e10
to
51b7841
Compare
|
@Zeliboba5 I rebased it on #29 now that there is nothing standing in the way anymore. This PR should be clean now. |
docs/models.rst
Outdated
| This code defines integer primary key for table. Sadly, currently only simple integer pk is supported. | ||
| This code defines integer primary key for table. | ||
| If you don't define a primary key, we will create a Integer primary key with name of ``id`` for you. | ||
| Sadly, currently only simple integer primary key is supported. |
There was a problem hiding this comment.
May be should force using 'id field at the moment? Current inner implementations using id all over the place and setting another name for pk will fall everything apart.
It made me realise why django needed separate pk field that stores reference to pk field
There was a problem hiding this comment.
I was having similar thoughts when I did this.
The reason I forced a default id fields was because of all the internal code using .id everywhere.
There was a problem hiding this comment.
Or should we copy Django, and have a pk field, and whatever has pk=True is just an alias to pk?
Or should we just stick to id?
There was a problem hiding this comment.
Then we should fix this description for now, because it's confusing and gives hope that you can use different name for pk. I created issue regarding this #36 and hope we will fix it before 1.0 because it's quite important in my opinion.
Could you also add some kind of validation that none other fields were defined with pk=True while we are forcing it ourselves
There was a problem hiding this comment.
Good point. I'll do that tonight.
|
@Zeliboba5 I updated the docs as you requested. |
|
👍 |
If a pk isn't defined, we create a pk called id for that model.
Also added Model method tests.
Based on #28 (which is based on #33). So please only merge this after those.