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

Implement astuple method #78

Closed
wants to merge 11 commits into from
Closed

Conversation

Insoleet
Copy link
Contributor

@Insoleet Insoleet commented Sep 6, 2016

Please checkout my proposal #77. Do not hesitate to comment this code.
The parameter tuple_factory is only tested with the tuple type. If you think this should be tested with other factories, do not hesitate to tell me.

@codecov-io
Copy link

codecov-io commented Sep 6, 2016

Current coverage is 100% (diff: 100%)

Merging #78 into master will not change coverage

@@           master   #78   diff @@
===================================
  Files           8     8          
  Lines         446   466    +20   
  Methods         0     0          
  Messages        0     0          
  Branches       97   105     +8   
===================================
+ Hits          446   466    +20   
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last update ca2a1e1...eebe654

"""
Tests for `asdict`.
"""
@given(st.sampled_from(IMMUTABLE_SEQ_TYPES))

This comment was marked as spam.

@Tinche
Copy link
Member

Tinche commented Sep 6, 2016

@hynek How do we feel about filter in astuple? My gut feeling is a little iffy since if you skip an element in the tuple, it fundamentally changes the meaning of the tuple. But maybe that's what you want sometimes.

@Tinche
Copy link
Member

Tinche commented Sep 7, 2016

Also I'm thinking we need either a dict_factory parameter or to have retain_collection_types to apply to dicts too, since right now astuple will map all OrderedDicts to dicts and there should be a way to avoid that.

I think just having retain_collection_types apply in the elif isinstance(v, dict): branch would be enough. (Use v.class as the dict factory.)

@hynek
Copy link
Member

hynek commented Sep 7, 2016

I very much think that filter makes sense. Especially in the use case with DB you may want to keep stuff out. Or drop a password field or something.

@Tinche
Copy link
Member

Tinche commented Sep 7, 2016

Yeah, let's leave it in. I was actually thinking about passwords in the first place. But the exact scenario I was worried about is impossible since no mandatory attributes are allowed after an attribute with a default value, so it's moot anyway.

@Insoleet
Copy link
Contributor Author

Insoleet commented Sep 7, 2016

I took all your comments into account.

called with the :class:`attr.Attribute` as the first argument and the
value as the second argument.
:param callable tuple_factory: A callable to produce tuples from. For
example, to produce ordered tuples instead of normal Python tuples.

This comment was marked as spam.

This comment was marked as spam.

@Tinche
Copy link
Member

Tinche commented Sep 7, 2016

Alright, we're getting close for Hynek to take a look. :)
Please add a changelog entry too, you can find examples in CHANGELOG.rst.

@Tinche
Copy link
Member

Tinche commented Sep 7, 2016

I've noticed retain_collection_type isn't propagated always in astuple. I've expanded our Hypothesis nested class strategy so tests can catch this, and written a test. I don't really feel like pasting it all in here, so let me issue a pull request onto your repository.

Tinche and others added 2 commits September 7, 2016 20:14
Expand the nested_class strategy. Test retain_collection_type in astu…
@Tinche
Copy link
Member

Tinche commented Sep 7, 2016

@hynek I think this is in good shape, please take a look when you have a chance.

@hynek
Copy link
Member

hynek commented Sep 8, 2016

could you please activate the shiny new maintainer collaboration feature? I don’t want to pester you with minutiae.

@Insoleet
Copy link
Contributor Author

Insoleet commented Sep 8, 2016

Done !

@hynek
Copy link
Member

hynek commented Sep 10, 2016

I guess this will need some work due to @Tinche’s work on simple_class?

@Tinche
Copy link
Member

Tinche commented Sep 10, 2016

Yeah, this needs a slight rebase. simple_classes should now be simple_classes(); i.e. the strategy is now parameterized.

@Insoleet
Copy link
Contributor Author

Ok, I fixed it.

@hynek hynek closed this in 1b419e3 Sep 11, 2016
@hynek
Copy link
Member

hynek commented Sep 11, 2016

Thank you!

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.

4 participants