Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Human readable choice labels #5

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Human readable choice labels #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGES
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
3.2.0 2015-07-20

[Jouni Kuusisto <[email protected]>]
* MINOR: Implemented choice labels for IdexableField subclasses.

3.1.1 2015-03-23

[Anton Berezin <[email protected]>]
Expand Down
13 changes: 13 additions & 0 deletions documentation/schema.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
.. _schema:
.. py:currentmodule:: resource_api

Schema
======
Expand Down Expand Up @@ -36,6 +37,18 @@ readonly (bool=False)
changeable (bool=False)
if True field can be set during creation but cannot be change later on. User's birth date is a valid example.

Field choices
"""""""""""""

For all fields inherit from :class:`schema.IndexableField`, it's possible to define list of possible
values as choices. When choices are defined for a field field value is only allowed to be set one of these, otherwise
:exc:`errors.ValidationError` is raised.

To define human readable labels for the value choices, pass a list or tuple of
labels as the *choice_labels* attribute for the field. This is useful for scenarios such as automated UI generation.

.. autoclass:: resource_api.schema.IndexableField

Primitive fields
----------------

Expand Down
24 changes: 22 additions & 2 deletions src/resource_api/schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,21 @@ def get_schema(self):


class IndexableField(BaseSimpleField):
"""
Base class for most of the primitive fields.

choices (list|tuple = None)
Predefined list of possible values for the field. ValidationError will be raised if field value doesn't
match any of the defined values.
choice_labels (list|tuple = None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea with human readable labels. But make it a dict then - not two list parameters. Cause I already foresee the situation when someone adds a choice somewhere in the middle of the list and forgets to add the label exactly at the same spot of another list - and WOOOHA! a month passes and no one has a clue why the data that was coming from the UI is so inappropriate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, for the primitive cases it would also make sense to have just a list of choices that would internally be transformed from [val1, val2, val3] into {val1:val1, val2:val2, val3:val3}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point and actually I had thought about it myself. My concern regarding the dict is the correct ordering of the choices: how can we guarantee correct ordering of the items? For some use cases like UI automation the correct ordering of choices could be crucial.

Order of items in python dict seems to be implementation specific. I see two options to address this issue:

  1. choices with labels defined as list of key-value pair tuples
  2. choices defined as enums

In both cases choices and labels would be split to separate lists in schema output, in order to preserve correct ordering (also on client side) and especially to maintain backward compatibility.

Somehow I dislike the idea of interpreting the contents of the choice list to guess whether it should include labels or not. Enum would support ordering and also deal with duplicate values.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then again, enum name as a label may be quite limiting. These values could be extended with extra label field, but meh..
Using OrderedDict would solve ordering but allow e.g. spaces in the label.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave your idea another thought.

Human labels just do not make sense with respect to Resource API. The choices that are mentioned in the current "choices" field must be the values that make sense for the client (including a human client).

The only scenario where you would hypothetically need both: short values and human readable title is in case if you want to "optimize bandwidth" so to say. If performance is to be kept in mind - Resource API is not an option. That has never been its design goal.

In case if you want to implement something like:

number_of_group_members = IntegerFIeld(
    choices=[12, 57, 187],
    choice_labels=["Small group count", "Medium group count", "Big group count"])

Please don't.

Do like:

group_type = StringField(choices=["Small", "Medium", "Big"])

And internally - i.e. in the implementation (not definition) of the individual resource do the mapping from the string values to the integers.

On the other hand, if in the example above you really need to expose both the size and a corresponding group type as human readable string - it is a clear indication that you should use a read-only Resource with a Link field.

Sth like:

class GroupType(Resource):

    count_name_map = {
        12: "Small",
        57: "Medium",
        187: "Big"
    }

    class Schema:
        count = IntegerField(description="Number of people in a group", pk=True)
        name = StringField(description="Human readable definition of a group")

    def exists(self, user, pk):
        return pk in self.count_name_map

    def get_data(self, user, pk):
        return self.count_name_map.get(pk)

    def get_uris(self, user, params=None):
        return self.count_name_map.keys()

    def get_count(self, user, params=None):
        return len(self.count_name_map)

...
# Somewhere in another resource

Links
    class group_type(Link):
        target = "Teacher"
        one_way = True
        cardinality = Link.cardinalities.ONE
        master = True

Thus I do not approve this commit :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave your idea another thought.

Human labels just do not make sense with respect to Resource API. The choices that are mentioned in the current "choices" field must be the values that make sense for the client (including a human client).

The only scenario where you would hypothetically need both: short values and human readable title is in case if you want to "optimize bandwidth" so to say. If performance is to be kept in mind - Resource API is not an option. That has never been its design goal.

In case if you want to implement something like:

number_of_group_members = IntegerFIeld(
    choices=[12, 57, 187],
    choice_labels=["Small group count", "Medium group count", "Big group count"])

Please don't.

Do like:

group_type = StringField(choices=["Small", "Medium", "Big"])

And internally - i.e. in the implementation (not definition) of the individual resource do the mapping from the string values to the integers.

On the other hand, if in the example above you really need to expose both the size and a corresponding group type as human readable string - it is a clear indication that you should use a read-only Resource with a Link field.

Sth like:

class GroupType(Resource):

    count_name_map = {
        12: "Small",
        57: "Medium",
        187: "Big"
    }

    class Schema:
        count = IntegerField(description="Number of people in a group", pk=True)
        name = StringField(description="Human readable definition of a group")

    def exists(self, user, pk):
        return pk in self.count_name_map

    def get_data(self, user, pk):
        return self.count_name_map.get(pk)

    def get_uris(self, user, params=None):
        return self.count_name_map.keys()

    def get_count(self, user, params=None):
        return len(self.count_name_map)

...
# Somewhere in another resource

class Links:
    class group_type(Link):
        target = "GroupType"
        one_way = True
        cardinality = Link.cardinalities.ONE
        master = True

Thus I do not approve this commit :)

List of human readable labels for defined choice values.
The length of the list must match length of the defined choices.
invalid_choices (list|tuple = None)
List of invalid values for the field. ValidationError will be raised if the field value matches any of the
defined values.
"""

def __init__(self, choices=None, invalid_choices=None, **kwargs):
def __init__(self, choices=None, choice_labels=None, invalid_choices=None, **kwargs):
super(IndexableField, self).__init__(**kwargs)

if choices is not None:
Expand All @@ -205,6 +218,12 @@ def __init__(self, choices=None, invalid_choices=None, **kwargs):
raise DeclarationError("[%d]: %s" % (i, str(e)))
choices = tempo

if choice_labels is not None:
if not isinstance(choice_labels, (list, tuple)):
raise DeclarationError("choices has to be a list or tuple")
if choices is None or len(choices) != len(choice_labels):
raise DeclarationError("length of choice_labels has to match with choices.")

if invalid_choices is not None:
if not isinstance(invalid_choices, (list, tuple)):
raise DeclarationError("invalid_choices has to be a list or tuple")
Expand All @@ -227,7 +246,7 @@ def __init__(self, choices=None, invalid_choices=None, **kwargs):
if inter:
raise DeclarationError("these choices are stated as both valid and invalid: %r" % inter)

self.choices, self.invalid_choices = choices, invalid_choices
self.choices, self.choice_labels, self.invalid_choices = choices, choice_labels, invalid_choices

def _validate(self, val):
super(IndexableField, self)._validate(val)
Expand All @@ -241,6 +260,7 @@ def _validate(self, val):
def get_schema(self):
rval = super(IndexableField, self).get_schema()
rval["choices"] = self.choices
rval["choice_labels"] = self.choice_labels
rval["invalid_choices"] = self.invalid_choices
return rval

Expand Down
Loading