Request config #2949
Request config #2949
Conversation
| for mapping in reversed(self._maps): | ||
| # reuses stored hash values if possible | ||
| d.update(mapping) | ||
| return iter(d) |
There was a problem hiding this comment.
It feels like you can have there the same iter(set().union(*self._maps)) instead of another dict construction.
There was a problem hiding this comment.
I did copy collections.ChainMap without careful thinking :)
There was a problem hiding this comment.
Let's keep it for sake of shared implementation with ChainMap
There was a problem hiding this comment.
Iteration is not the most common call, get/__getitem__ doesn't create a dict.
Moreover your proposal constructs a set, not sure what is better.
aiohttp/helpers.py
Outdated
| @@ -741,3 +742,63 @@ def set_result(fut, result): | |||
| def set_exception(fut, exc): | |||
| if not fut.done(): | |||
| fut.set_exception(exc) | |||
|
|
|||
|
|
|||
| class ChainedProps(Mapping): | |||
There was a problem hiding this comment.
I think the ChainedMaps would be better name here since it's a mapping. Props suffix leaves the feeling that it's something about attr-based access. While ChainedMaps causes collision with stdlib ChainMap, still, they have a bit different naming what should cause suspicions. However, if we make this class for internal-only usage, than own ChainMap is fine.
There was a problem hiding this comment.
Naming is the hardest thing!
Maybe we can invite a name without chain word?
There was a problem hiding this comment.
Hm...How about to just a Mapping? With the trick that all the maps will get merged right at the moment of it creation?
There was a problem hiding this comment.
I mean, if we do merge all the maps, than this class has no reason to exists. The merge result could be returned as MappingProxy - well known immutable mapping.
There was a problem hiding this comment.
Let's name it ChainMapProxy and keep existing implementation -- I don't like early merging.
aiohttp/helpers.py
Outdated
|
|
||
| def __repr__(self): | ||
| content = ", ".join(map(repr, self._maps)) | ||
| return 'ChainedProps({})'.format(content) |
There was a problem hiding this comment.
Here we should reuse class name in repr to avoid any confusions with subclasses. I know, there wouldn't any subclasses of those, but still this little bit could save few debugging hours for some hacker.
There was a problem hiding this comment.
I don't want to allow subclassing. Hmm, should do it explicitly.
aiohttp/web_request.py
Outdated
|
|
||
| @property | ||
| def config(self): | ||
| return Namespace(self.config_dict) |
There was a problem hiding this comment.
I'm in a very doubt if we need not a mapping class here. This will cause two problems:
- Inconsistent access to config values. While names are valid Python attributes it's all fine, but how to access to a value with
mycomponent.cache? Here, user have to fallback togetattr(request.config, 'mycomponent.cache'). - Linters. Linters will complain about accessing to unknown class attributes. This will be very annoying.
I would prefer to have one way to work with the config that works and right. So far, config_dict is more powerful and useful interface to it.
There was a problem hiding this comment.
I heard complains many times that IDE doesn't autocomplete string keys but does it for custom attributes.
There was a problem hiding this comment.
Yes, they don't do keys autocomplete unless you explicitly define those keys in the same scope. However, their ability to autocomplete custom attributes is limited and based on type knowledge and similar usages.
However, for pylint you will have to put Namespace class into ignored-classes since it will think that it's your fault since you're referencing to unknown class instance attribute.
There was a problem hiding this comment.
@socketpair I recall you wanted the feature.
Please explain your opinion
|
Silence. |
|
@asvetlov |
|
Done |
Codecov Report
@@ Coverage Diff @@
## master #2949 +/- ##
==========================================
+ Coverage 97.99% 98.08% +0.09%
==========================================
Files 40 40
Lines 7523 7622 +99
Branches 1318 1350 +32
==========================================
+ Hits 7372 7476 +104
+ Misses 48 47 -1
+ Partials 103 99 -4
Continue to review full report at Codecov.
|
What do these changes do?
Add
request.configandrequest.config_dictproperties.Both props do lookup into parent app if the name is not found in sub-application.
.configis a namespace (req.config.key) but.config_dictis a mapping (req.config_dict['key']).Both structures are immutable, to setup config value use
app['key'] = valon application initialization stage.Are there changes in behavior for the user?
The change is backward compatible but two new attributes are added to request instance.
Related issue number
#2689
Checklist
CONTRIBUTORS.txtCHANGESfolder<issue_id>.<type>for example (588.bugfix)issue_idchange it to the pr id after creating the pr.feature: Signifying a new feature..bugfix: Signifying a bug fix..doc: Signifying a documentation improvement..removal: Signifying a deprecation or removal of public API..misc: A ticket has been closed, but it is not of interest to users.