django-polymorphic / django-polymorphic-tree

Polymorphic MPTT tree support for models
Other
170 stars 45 forks source link

Support for UUID primary key field #52

Closed njamaleddine closed 7 years ago

njamaleddine commented 8 years ago

It looks like models that use UUIDField as the primary key are not supported because of various places where an id is being cast to an int. Would it make sense to cast to a pass a string from the client and parse it on the server as an int or leave it as a string if there's a ValueError?

I'm willing to refactor and clean things up if this is a possible path for our solution, right now this is more like patchwork for getting support for UUID primary keys on my own application. And if it's worth going this direction, I'll squash and rebase my commits into a single commit for this pull request.

Any thoughts would be appreciated, thanks! 👍

codecov-io commented 8 years ago

Current coverage is 45.32%

Merging #52 into master will decrease coverage by 0.17%

@@             master        #52   diff @@
==========================================
  Files            11         11          
  Lines           600        609     +9   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            273        276     +3   
- Misses          327        333     +6   
  Partials          0          0          

Powered by Codecov. Last updated by 5b5d58e...aef7a7e

vdboor commented 8 years ago

Thanks for the initial work on this!

Could you move the try.catch logic to a function that handles this, so the method stays clean? Perhaps, reading self.model._meta could even be better to see whether the PK field is an integer or UUID, but this is a nice to have.

I'd also love to see some tests on this, otherwise I might risk breaking your feature on future changes.

njamaleddine commented 8 years ago

Great, I will keep you updated. Also will look into making it accept a PK of any type not just patching on UUID

vdboor commented 7 years ago

I've merged this branch now! Thanks for the work!