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

Introduce attribute metadata. #96

Merged
merged 13 commits into from Nov 19, 2016

Conversation

@Tinche
Copy link
Member

@Tinche Tinche commented Sep 21, 2016

Alright, here's a first pass, open for comments. Docs will come after the implementation is settled.

The syntax is:

@attr.s
class A:
    a = attr.ib(metadata=<metadata>)

where <metadata> can be:

  • None
  • a mapping (technically something that can be passed to the dict constructor)

On Python 2, the metadata (if not None) will be exposed as a plain dict. On Python 3, the original, passed-in mapping will be wrapped in a https://docs.python.org/3.5/library/types.html#types.MappingProxyType.

I'm not entirely happy with None as the default here (it's my pet peeve to have Optional[Collection] instead of just an empty collection) but I feel an empty dict per attribute might be too much memory overhead? MappingProxyType would have solved this elegantly (we would have one empty instance, used everywhere where there was no metadata), but alas.

I've tried marking the metadata field as excluded from hashing but I don't think we care much about that. I put together a property test for this but it kept failing since the default field isn't excluded, and the default was getting set to {}. Guess we don't care about it too much? Doubt many people will make sets of Attributes.

As a bonus I've made _CountingAttr slotted. There's no reason it shouldn't be.

@hynek
Copy link
Member

@hynek hynek commented Sep 24, 2016

FTR I’m mostly fine with the approach and am waiting for feedback from other people that wanted metadata in #63 most notably @wearpants :)

Loading

@wearpants
Copy link

@wearpants wearpants commented Sep 24, 2016

Looks basically like what I'm after :-)

Though I'd agree with @Tinche's instinct that metadata should always be a mapping even when empty - memory overhead for an extra dict doesn't seem like a big deal to me, it's per-class attr-using class not per instance (and will generally stay out of cache lines if unused, and maybe even get swapped out if you're lucky with page layouts). Could use MappingProxy conditionally if it's available but probably overkill.

Loading

@hynek
Copy link
Member

@hynek hynek commented Sep 24, 2016

FWIW we could define a dict-like singleton and be done with it.

Loading

@wearpants
Copy link

@wearpants wearpants commented Sep 24, 2016

Hmm, singleton would mean metadata is read-only after creation - that what we want?

Loading

@hynek
Copy link
Member

@hynek hynek commented Sep 24, 2016

I thought that’s the plan but in any case it’s better to start stricter and loosen up if necessary. Unless someone has some great example why we mutability?

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Sep 24, 2016

OK, let's agree that an empty dict is a preferable API than an Optional dict. I'll play around and whip something up.

@wearpants Yeah, attributes are immutable and metadata as part of an attribute should be immutable, ideally.

Loading

@wearpants
Copy link

@wearpants wearpants commented Sep 24, 2016

Wfm

Loading

@codecov-io
Copy link

@codecov-io codecov-io commented Sep 24, 2016

Current coverage is 100% (diff: 100%)

Merging #96 into master will not change coverage

@@           master   #96   diff @@
===================================
  Files           8     8          
  Lines         466   496    +30   
  Methods         0     0          
  Messages        0     0          
  Branches      105   107     +2   
===================================
+ Hits          466   496    +30   
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last update cbfb229...3c10c46

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Sep 24, 2016

Here, metadata is immutable on Python 2 now too (best effort basis, via a simple immutability wrapper in _compat.py). We use a singleton for empty metadata now.

When I was testing this, instead of adapting the simple_classes Hypothesis strategy already in use, to sometimes include metadata, I defined another strategy just for metadata tests. This was a dumb decision! If I hadn't done that, I'd have found out earlier that MappingProxyType on Python 3 breaks pickle. (That's fixed too.)

Loading

@hynek
Copy link
Member

@hynek hynek commented Sep 25, 2016

I have so minor stuff but would like to settle on the big picture first. @glyph you OK with this?

Loading

@glyph
Copy link
Contributor

@glyph glyph commented Sep 27, 2016

@hynek The mappingproxy boilerplate makes me a bit sad, but I guess it's a total implementation detail, so that's fine.

Rather than allowing None, why not just set the default to {} and then do the mappingproxy construction in _CountingAttr rather than attr?

Also, I would very much like to see some information on both setting and getting metadata in the docs, if the feature is going to be added. Writing the docs before adding a controversial feature can often uncover weird little edge cases. I don't think that it will do so in this case, since just looking up in a dict is so simple - but I always think it's not going to do so until I do it!

Loading

@hynek
Copy link
Member

@hynek hynek commented Sep 28, 2016

I guess what glyph is saying: @Tinche pls finalize. :)

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Oct 4, 2016

I've been a little sick these past few days, hence the slow progress.

While working on the docs, this style of metadata composability came up. What do we think?

# Library one.
my_type_metadata = '__my_type_metadata'

def typed(cls, default=attr.NOTHING, validator=None, repr=True, cmp=True, hash=True, init=True, convert=None, metadata={}):
    metadata = dict() if not metadata else metadata
    metadata[my_type_metadata] = cls
    return attr.ib(default, validator, repr, cmp, hash, init, convert, metadata)

# Library two.
my_json_metadata = '__my_json_metadata'

def json_name(name, default=attr.NOTHING, validator=None, repr=True, cmp=True, hash=True, init=True, convert=None, metadata={}):
        if isinstance(default, attr._CountingAttr):
            # Wrapping a prepared _CountingAttr instance.
            metadata = dict() if not default.metadata else default.metadata
            metadata[my_json_metadata] = name
            return default
        else:
            # Wrapping ``attr.ib()``.
            metadata = dict() if not metadata else metadata
            metadata[my_json_metadata] = cls
            return attr.ib(default, validator, repr, cmp, hash, init, convert, metadata)

# The user code.
@attr.s
class C(object):
    x = json_name('X', typed(int, default=1, init=False))
    y = json_name('Y', default='A')
attr.fields(C).x.metadata[my_type_metadata]
<class 'int'>
attr.fields(C).x.metadata[my_json_metadata]
'X'

Hot or not? Top or flop?

Loading

@glyph
Copy link
Contributor

@glyph glyph commented Oct 4, 2016

  1. I like the idea.
  2. _CountingAttr is private, so, that's not great.
  3. We can only stack these in one order, typed, then, json_name, which means that some random library can define "extension" metadata in a way that doesn't compose with another library which also expects to be at the "bottom" of the wrapper-stack.
  4. I'd probably go with *optional_wrapped_attr, and check its length, rather than multi-purposing default

Anyway, it's a promising direction :)

Loading

@wearpants
Copy link

@wearpants wearpants commented Oct 4, 2016

Is this something that attrs should be supporting directly or is this up to
users to implement? I can see a role for providing general helpers for
composition, but not sure attrs should provide implementation for specific
use cases like JSON... Reminds me a bit of the canonical code snippets in
itertools docs.

But yeah this looks promising

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Oct 13, 2016

I was working on the docs and I wanted to put a pattern for defining attribute wrappers under Extending, and this came up. I'll use the second type :)

Loading

@hynek
Copy link
Member

@hynek hynek commented Oct 13, 2016

So I’m back from ZA; did I miss something? :)

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Oct 13, 2016

I've added some docs. I think recommending a composable attr.ib wrapper like I mentioned can wait for another PR since there are additional changes (and discussion) required for it.

Loading

@hynek
Copy link
Member

@hynek hynek commented Oct 14, 2016

Before I go into details:

  1. semantic new lines
  2. double quotes
  3. """\ndocstring\n"""

please :)

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Oct 16, 2016

@hynek I've addressed the docstrings and double quotes. I don't really understand semantic newlines in docs so you might have to swoop in and do some formatting.

Loading

@hynek
Copy link
Member

@hynek hynek commented Oct 17, 2016

Semantic newlines just means to press enter after each sentence. ;) On a first glance it looks fine.

Final RfC for @glyph and @wearpants is hereby open. I’ll try to implement something I had in mind with it to check its usability.

Loading

@hynek hynek added this to the 16.3.0 milestone Oct 17, 2016
@hynek hynek added this to the 16.3.0 milestone Oct 17, 2016
@wearpants
Copy link

@wearpants wearpants commented Oct 17, 2016

Will look over in next day or two

On Oct 17, 2016 1:57 AM, "Hynek Schlawack" notifications@github.com wrote:

Semantic newlines just means to press enter after each sentence. ;) On a
first glance it looks fine.

Final RfC for @glyph https://github.com/glyph and @wearpants
https://github.com/wearpants is hereby open. I’ll try to implement
something I had in mind with it to check its usability.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#96 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAe2AKKnc-vi-X8jV7wr_09DaMAK291Fks5q0w5TgaJpZM4KCSuY
.

Loading

@wearpants
Copy link

@wearpants wearpants commented Oct 19, 2016

Looks good to me :-)

Loading

@hynek
Copy link
Member

@hynek hynek commented Nov 9, 2016

First of all: @Tinche I’m super sorry I didn’t get around earlier.

But.

On this rather sad day in history, I’m kind of excited, because this is what I did: https://gist.github.com/hynek/78fef47fea3cbc6b683cd8ab0fb68567

I used the meta data feature to implement a declarative way for having configuration via env variables!

First file is client side usage, middle file is implementation, and final file is program output.

I’d love to hear some thoughts on this!


There is one question tho: if we add attribute meta data, wouldn’t it also make sense to add class meta data? In my example I could put most of the arguments of environ_to_app_config() into some class meta data for an even nicer experience.

(Sorry for disgusting legacy Python; I used it within a Twisted app that can’t be ported yet.)

Loading

@wearpants
Copy link

@wearpants wearpants commented Nov 9, 2016

Insufficiently cadfineated to understand the implementation, but THAT'S AWESOME @hynek.

Not sure I follow on class metadata - couldn't regular class vars do that?

Might be nice to have an attr.contrib package for including batteries like this (or perhaps a recipe collection somewhere)

Loading

@hynek
Copy link
Member

@hynek hynek commented Nov 9, 2016

I’ve updated it and now I’m reasonably excited as much as I can get excited about software while being depressed about the world.

Just for posterity, this is what application code boils down to:

from app_config import (app_config, env_var, env_var_from_vault,
                        env_var_group, environ_to_app_config)


@app_config(prefix="APP", vault_prefix="WHOIS_{env}")
class WhoisConfig:
    @app_config
    class Prometheus:
        address = env_var(default="127.0.0.1")
        port = env_var(default="0")
        consul_token = env_var_from_vault()

    env = env_var()
    prometheus = env_var_group(Prometheus)


if __name__ == '__main__':
    ac = environ_to_app_config(WhoisConfig)

    print(ac)

and it gives you this:

$ env APP_ENV=dev APP_PROMETHEUS_PORT=7000 SECRET_WHOIS_DEV_APP_PROMETHEUS_CONSUL_TOKEN=abc python app.py
WhoisConfig(env='dev', prometheus=WhoisConfig.Prometheus(address='127.0.0.1', port='7000', consul_token='abc'))

Loading

Copy link
Member

@hynek hynek left a comment

Almost there.

Loading

>>> attr.fields(C).x.metadata['my_metadata']
1
Metadata is currently not used by ``attrs``, and is generally meant to enable rich functionality in third-party libraries.

This comment has been minimized.

Loading


- To avoid metadata key collisions, consider exposing your metadata keys from your modules.::

from mylib import my_metadata_key

This comment has been minimized.

Loading


.. doctest::

>>> my_type_metadata = '__my_type_metadata'

This comment has been minimized.

Loading

@hynek
Copy link
Member

@hynek hynek commented Nov 10, 2016

Oh and Changelog please. :)

16.3.0 goes out once this is merged.

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Nov 11, 2016

Will get around to this today/tomorrow :)

Loading

@Tinche Tinche force-pushed the feature/attribute-metadata branch from 1587d36 to cb743cd Nov 15, 2016
hynek
hynek approved these changes Nov 15, 2016
@hynek
Copy link
Member

@hynek hynek commented Nov 15, 2016

flake8 is failing

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Nov 16, 2016

Huh, flake8 put out a new release and now it wants two blank lines after class definitions too :)

Anyway, I addressed all your points except what to use as metadata keys. My gut says strings are better since the attribute repr will be nicer and I don't think clashes will be a big deal in practice. Also that one person needed to serialize attributes, don't know how this would play out if using object() instances as keys. I don't feel that strongly about it, so please feel free to change the docs yourself if you want to. :)

Loading

@@ -13,6 +13,8 @@ Changes:

- Don't overwrite ``__name__`` with ``__qualname__`` for ``attr.s(slots=True)`` classes.
`#99 <https://github.com/hynek/attrs/issues/99>`_
- Attributes can now have metadata dictionaries.

This comment has been minimized.

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Nov 17, 2016

Deal. I also moved it up since it's more interesting than a qualname bugfix :)

Loading

@hynek
Copy link
Member

@hynek hynek commented Nov 18, 2016

I’m confident like this it's grammatically wrong: either drop attrs for good or make it possessive (“attrs’s” or “attrs’”).

Loading

@Tinche
Copy link
Member Author

@Tinche Tinche commented Nov 19, 2016

Tweaked it :)

Loading

hynek
hynek approved these changes Nov 19, 2016
@hynek hynek merged commit 0fac921 into python-attrs:master Nov 19, 2016
3 checks passed
Loading
@hynek
Copy link
Member

@hynek hynek commented Nov 19, 2016

Thanks everyone!

Loading

@Tinche Tinche deleted the feature/attribute-metadata branch Jan 20, 2017
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

5 participants