Skip to content

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.

@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
Contributor

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.

@lovelydinosaur
Copy link
Contributor

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

@xordoquy
Copy link
Contributor

Note: by dict we should read any mapping type.

@lovelydinosaur
Copy link
Contributor

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
Contributor

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
Contributor

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

@pattisdr
Copy link
Contributor Author

Tests added.

@lovelydinosaur
Copy link
Contributor

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.

4 participants