Skip to content

ListField should enforce that input is a list#3513

Merged
lovelydinosaur merged 6 commits intoencode:masterfrom
pattisdr:feature/ListField_needs_to_enforce_list
Nov 18, 2015
Merged

ListField should enforce that input is a list#3513
lovelydinosaur merged 6 commits intoencode:masterfrom
pattisdr:feature/ListField_needs_to_enforce_list

Conversation

@pattisdr
Copy link
Copy Markdown
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
Copy Markdown
Contributor

thedrow commented Oct 16, 2015

What about sets or tuples?

@pattisdr
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

@xordoquy
Copy link
Copy Markdown
Contributor

Note: by dict we should read any mapping type.

@lovelydinosaur
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@foresmac
Copy link
Copy Markdown

Probably want tests that prove this works as intended.

@xordoquy
Copy link
Copy Markdown
Contributor

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

@pattisdr
Copy link
Copy Markdown
Contributor Author

Tests added.

@pattisdr pattisdr closed this Nov 16, 2015
@pattisdr pattisdr reopened this Nov 16, 2015
@lovelydinosaur
Copy link
Copy Markdown
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.

5 participants