Skip to content

Conversation

njamaleddine
Copy link
Contributor

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

codecov-io commented Jun 2, 2016

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

vdboor commented Jun 3, 2016

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
Copy link
Contributor Author

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

@vdboor vdboor merged commit 48dde42 into django-polymorphic:master Jan 11, 2017
@vdboor
Copy link
Member

vdboor commented Jan 11, 2017

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

vdboor added a commit that referenced this pull request Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants