Only integer PK fields currently supported; str or uuid pk throws NoReverseMatch exception

[phoikoi]

From reading the source at the end of views.py, it seems this is a known problem. But I thought I'd surface it in an issue as I am working on a possible solution for it, which can be seen here: phoikoi@3efbded . I have not made a pull request yet as I don't have tests written for it. But just as an idea jumping off point, perhaps it might be useful even now. (Note: I would recommend leaving FloatField pk support for a later version.) ;)

[carltongibson]

Hey @phoikoi 👋

Yes... it is a TODO. I've been pondering various options.

In the name of simplicity, I'm leaning towards a straight attribute here, maybe lookup_url_converter, that would default to int, to go with the other related attributes:

neapolitan/src/neapolitan/views.py

Lines 77 to 78 in 16c1259

	lookup_field = "pk"
	lookup_url_kwarg = None

Note, lookup_url_kwarg should already be being used by get_urls(), but isn't (yet). 🧐

My suspicion is that the kind of mapping logic you've sketched would forever ramify, for not much gain. (Beyond the different types in play, I frequently use custom converters, and there's no way for Neapolitan to know about those.) A simple attribute, which could be injected via the as_view() call, avoids all that...

class MyUUIDModelView(CRUDView):
    model = MyUUIDModel
    lookup_url_kwarg = 'uuid'  # Maybe you set this too 🤔
    lookup_url_converter = 'uuid'

What do you think?

[phoikoi]

Yes, I definitely see how it could get very branchy. I did briefly look at the converters code in Django, and it appears you can get a list of all default and registered converters via django.urls.converters.get_converters(), so perhaps the code could find custom converters too? It just seems more dry to not have to manually set parameters if the code can discover them. On the other hand, explicit is better than implicit, or so I've heard, so... there are pros/cons all round. I might keep dinking around with the automatic approach in my spare time.

But if the attribute route seems more orthogonal to your intended design or has other salient features that turn up, I won't complain about it being the Chosen Path. :)

[carltongibson]

😜

• I think it's likely a lot of (likely unreliable) code to replace a simple assignment (which can always be on a base class if we're concerned about DRY, which we might not be).
• I haven't ruled out a more programatic approach, but I'd likely want to see it injected into the as_view()/get_urls() calls (maybe via a Role subclass 🤫) rather than being a hardcoded switch… — still thinking.

If you wanted to make a PR adding the attribute, and making use of lookup_url_kwarg, that would be cool. 😎 (No stress though 🥳)

[carltongibson]

Fixed in 24.4