bigjason / django-choices

Sanity to the django choices functionality.
MIT License
175 stars 23 forks source link

DjangoChoices.labels does not do what it's documented for #54

Closed w00kie closed 5 years ago

w00kie commented 6 years ago

Hello, I'm on django-choices==1.6.0 and encounter an issue with DjangoChoices.label. The documentation says it should return a dictionary mapping labels to values. What I get is a dictionary mapping property to specified label.

With the following in my model:

class DevStatus(DjangoChoices):
    na = ChoiceItem('NA', 'NA')
    ready = ChoiceItem('RDY', 'Ready For Development')
    specs_to_discuss = ChoiceItem('TBD', 'Specs To Discuss')
    in_progress = ChoiceItem('INP', 'In Progress')
    partially_delivered = ChoiceItem('PAR', 'Partially Delivered')
    completed = ChoiceItem('COM', 'Completed')

I get this result:

>>> PER.DevStatus.labels
{'in_progress': 'In Progress', 'partially_delivered': 'Partially Delivered', 'specs_to_discuss': 'Specs To Discuss', 'completed': 'Completed', 'ready': 'Ready For Development', 'na': 'NA'}

Where what I'm expecting would be:

>>> PER.DevStatus.labels
{'In Progress': 'INP', 'Partially Delivered': 'PAR', 'Specs To Discuss': 'TBD', 'Completed': 'COM', 'Ready For Development': 'RDY', 'NA': 'NA'}
sergei-maertens commented 6 years ago

Thanks for reporting this, you're right, the documentation is wrong on this and shall be updated :)

On Dec 19, 2017 04:48, "François Rejeté" notifications@github.com wrote:

Hello, I'm on django-choices==1.6.0 and encounter an issue with DjangoChoices.label. The documentation says it should return a dictionary mapping labels to values. What I get is a dictionary mapping property to specified label.

With the following in my model:

class DevStatus(DjangoChoices): na = ChoiceItem('NA', 'NA') ready = ChoiceItem('RDY', 'Ready For Development') specs_to_discuss = ChoiceItem('TBD', 'Specs To Discuss') in_progress = ChoiceItem('INP', 'In Progress') partially_delivered = ChoiceItem('PAR', 'Partially Delivered') completed = ChoiceItem('COM', 'Completed')

I get this result:

PER.DevStatus.labels {'in_progress': 'In Progress', 'partially_delivered': 'Partially Delivered', 'specs_to_discuss': 'Specs To Discuss', 'completed': 'Completed', 'ready': 'Ready For Development', 'na': 'NA'}

Where what I'm expecting would be:

PER.DevStatus.labels {'In Progress': 'INP', 'Partially Delivered': 'PAR', 'Specs To Discuss': 'TBD', 'Completed': 'COM', 'Ready For Development': 'RDY', 'NA': 'NA'}

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bigjason/django-choices/issues/54, or mute the thread https://github.com/notifications/unsubscribe-auth/AFQ01ucDyhYcx4Lb3ctapMezB51C8tJmks5tBzHygaJpZM4RGZ6A .

w00kie commented 6 years ago

So there is no function or property that would give me the mapping I expect? I ended up taking the values property and reversing the keys/values to get things working but ideally I would expect to have something provided directly by the library.

What I have is a Django app that syncs some of its data from outside APIs through a custom manage.py command. The labels for the choices in the model declaration fit the ones returned by the API but I need to translate these labels into the proper values to create or update the model instances.

sergei-maertens commented 6 years ago

Actually, looking at the tests, it seems to be the intended behaviour is the one documented in the docs, so this would be a bug then. Only things is that I'm not sure what happens if you have the same label for different choice items - that would have a key collission, though I guess we can solve it like https://github.com/bigjason/django-choices/blob/master/djchoices/choices.py#L45

Having a mapping from attribute to value makes no sense in this context, since the DjangoChoices (sub)class already does that.

w00kie commented 6 years ago

Using my example, I don't see how I could instantiate an object with a devstatus of In Progress. I don't see a way using just the things supplied by the library to translate In Progress into INP.

I end up inversing the mapping myself with this:

devstatus_choices = {v: k for k, v in PER.DevStatus.values.items()}
value = devstatus_choices['In Progress']

But I would hope this would be provided by the library itself.

sergei-maertens commented 6 years ago

Yes, as I said, it looks like a bug, so it will be fixed. I just have to make sure there's no breakage of existing code.

On Dec 19, 2017 09:10, "François Rejeté" notifications@github.com wrote:

Using my example, I don't see how I could instantiate an object with a devstatus of In Progress. I don't see a way using just the things supplied by the library to translate In Progress into INP.

I end up inversing the mapping myself with this:

devstatus_choices = {v: k for k, v in PER.DevStatus.values.items()} value = devstatus_choices['In Progress']

But I would hope this would be provided by the library itself.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bigjason/django-choices/issues/54#issuecomment-352669638, or mute the thread https://github.com/notifications/unsubscribe-auth/AFQ01mcpqXfzgVwL0N6-8WX310t8TlR1ks5tB29ugaJpZM4RGZ6A .

sergei-maertens commented 6 years ago

So, I've finally found some time and motivation to look into this. My conclusion - going of the tests - is that the documentation was wrong, so I've updated that. As for what you actually want, I'm a bit confused:

  1. Why do you want to map labels to values? The intended usage is doing something like mymodel.choice = Choices.some_attribute, which assigns the correct value. Human-readable label is derived from the value.

  2. There is actually a risk - if you're using (translatable) labels, you may end up having the same human readable label for different values. It's not possible to do the reverse mapping in that case. While having the same representation of different values is an odd thing in itself, it may be a valid use case in simplified UX-es.

So, I'm not yet building something into the library to get this reverse mapping, until it's clear what would be the actual use case for it. Could you provide some more context?

edit: saw the context in your earlier report. I understand the usage, but reverse mapping .values() is probably your best approach here. It's stable API, so we won't just casually break it.

pacahon commented 6 years ago

@sergei-maertens Is it possible to update the documentation on readthedocs? Thx.

sergei-maertens commented 5 years ago

I've released 1.6.2 just now, this should also kick off a new build on readthedocs. Please let me know if something still went wrong!