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

Sets not serialized by fields.List #175

Merged
merged 6 commits into from
Jan 14, 2014
Merged

Sets not serialized by fields.List #175

merged 6 commits into from
Jan 14, 2014

Conversation

kdeldycke
Copy link
Contributor

The List field can't serialize a set(). I think it should.

Here is a unit test providing the proof, and an attempt to fix this issue.

@@ -59,7 +59,7 @@ def to_marshallable_type(obj):
if obj is None:
return None # make it idempotent for None

if hasattr(obj, '__getitem__'):
if hasattr(obj, '__iter__'):
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this change is needed. This code is used in FormattedString and Url to ensure we have a mapping that can be used with kwargs to format the string/url, and should specifically check that the object passed has dict-like behavior. Your set test passes without it.

@skimbrel
Copy link
Contributor

Aside from the issue with to_marshallable_type this looks good to me. Let me know if there's something I'm missing in that specific part. If not, I'll merge this minus that line. Thanks for the PR!

@kdeldycke
Copy link
Contributor Author

@skimbrel : you're right. I changed it thinking it was the expected behaviour but I was wrong. I reverted my change. Thanks for the review ! :)

@skimbrel
Copy link
Contributor

Thanks for the update! Merging.

skimbrel added a commit that referenced this pull request Jan 14, 2014
@skimbrel skimbrel merged commit 6375b41 into flask-restful:master Jan 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants