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.

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

ListField should enforce that input is a list #3513

Merged

Conversation

pattisdr
Copy link
Contributor

Purpose

A dictionary is allowed as input in a ListField. If a dictionary is received as input, the keys of the object are retained, and the values are ignored. I assume the ListField should enforce that the input is an array, especially because the error message in to_internal_value includes "not a list" if something is wrong with the data.

Changes

Enforces that data cannot be a collections.Mapping.

@thedrow
Copy link
Contributor

thedrow commented Oct 16, 2015

What about sets or tuples?

@pattisdr
Copy link
Contributor Author

I could include tuples and sets

if not isinstance(data, (list, tuple, Set)):
    self.fail('not_a_list', input_type=type(data).__name__)

@xordoquy xordoquy added this to the 3.3.0 Release milestone Oct 16, 2015
@xordoquy
Copy link
Collaborator

Thanks for the path.
I don't think it should force to list / tuple / set. Any iterable should do.
The key issue here is that a dictionary is an iterable on the keys.
Questions to be answered:

  • do we really want to limit the valid type to a limited set ?
  • can't we just exclude dictionaries from the ListField ?

@pattisdr
Copy link
Contributor Author

You're right, I think excluding dictionaries from the ListField in addition to what was already in that line would do the trick.

@tomchristie
Copy link
Member

Agree - excluding dicts sounds like a good way around to do this.

@xordoquy
Copy link
Collaborator

Note: by dict we should read any mapping type.

@tomchristie
Copy link
Member

Note: by dict we should read any mapping type.

IDK - any chance that could be a bit fuzzy in cases we've not thought of?
No strong opinion tho'

@xordoquy
Copy link
Collaborator

Well, collections.Mapping should be a good base class to test against (as opposed to just dict).

@foresmac
Copy link

Probably want tests that prove this works as intended.

@xordoquy
Copy link
Collaborator

Yup, having tests would be great so we merge it.

@pattisdr
Copy link
Contributor Author

Tests added.

@pattisdr pattisdr closed this Nov 16, 2015
@pattisdr pattisdr reopened this Nov 16, 2015
@tomchristie
Copy link
Member

Looks good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants