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

asdict - propagate dict_factory properly #45

Merged
merged 4 commits into from Jun 2, 2016
Merged

asdict - propagate dict_factory properly #45

merged 4 commits into from Jun 2, 2016

Conversation

@Tinche
Copy link
Member

@Tinche Tinche commented May 26, 2016

So yeah, I made a dumb mistake when adding support for dict_factory - I forgot to ensure the factory propagates further than the initial class - to attributes containing dicts, lists and other instances of attrs classes. It came up instantly at work when we upgraded to actually use the feature :) My bad.

I'm pretty sure the dict_factory argument should propagate - one of the use cases is to use an OrderedDict to preserve order, and that's just not going to happen if ordinary dicts are used anywhere in the conversion.

The fix is simple, just propagate the argument in asdict.

Since the original tests didn't catch this, I've worked on expanding our Hypothesis strategies. (More challenging than just the fix :) In addition to the simple_classes strategy, there is also now a nested_classes strategy. nested_classes will generate attrs classes just like simple_classes, but attributes may also contain the following default values:

  • instances of other nested_classes attrs classes
  • lists, containing a single element pulled from nested_classes
  • dicts, containing a key "cls" pointing to a class pulled from nested_classes

These cases should cover all cases in asdict. There's a new test, test_recurse_property, which asserts that for any nested_classes class, all dicts produced by asdict are of the appropriate class.

@hynek
Copy link
Member

@hynek hynek commented Jun 2, 2016

I guess we need to remove hypothesis health checks for PyPy?

https://travis-ci.org/hynek/attrs/jobs/133256124

@Tinche
Copy link
Member Author

@Tinche Tinche commented Jun 2, 2016

Huh, weird. Let me install PyPy and see what's going on when I get a free moment.

@hynek
Copy link
Member

@hynek hynek commented Jun 2, 2016

It’s just too slow on Travis. Just disable the health check on PyPy.

@codecov-io
Copy link

@codecov-io codecov-io commented Jun 2, 2016

Current coverage is 100%

Merging #45 into master will not change coverage

  1. File src/attr/_make.py (not in diff) was modified. more
@@             master        #45   diff @@
==========================================
  Files             7          7          
  Lines           367        368     +1   
  Methods           0          0          
  Messages          0          0          
  Branches         86         86          
==========================================
+ Hits            367        368     +1   
  Misses            0          0          
  Partials          0          0          

Powered by Codecov. Last updated by e83f0e5...a063c56

@Tinche
Copy link
Member Author

@Tinche Tinche commented Jun 2, 2016

There we go. Thought I stumbled upon a Hypothesis bug for a while, but it was just Tox not propagating environment vars. :)

@hynek hynek merged commit d10e5c4 into python-attrs:master Jun 2, 2016
2 checks passed
2 checks passed
codecov/project 100% compared to e83f0e5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hynek
Copy link
Member

@hynek hynek commented Jun 2, 2016

thanks!

@Tinche
Copy link
Member Author

@Tinche Tinche commented Jun 2, 2016

Yay \o/

@Tinche Tinche deleted the Tinche:bugfix/dict-factory-recursive branch Jun 2, 2016
hynek added a commit that referenced this pull request Jun 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants