Use attrs library for data classes #2690

asvetlov · 2018-01-25T22:38:53Z

No description provided.

codecov-io · 2018-01-25T23:28:57Z

Codecov Report

Merging #2690 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2690      +/-   ##
==========================================
+ Coverage   98.01%   98.01%   +<.01%     
==========================================
  Files          39       39              
  Lines        7245     7270      +25     
  Branches     1258     1264       +6     
==========================================
+ Hits         7101     7126      +25     
  Misses         44       44              
  Partials      100      100

Impacted Files	Coverage Δ
aiohttp/web_request.py	`99.68% <100%> (ø)`	⬆️
aiohttp/helpers.py	`97.98% <100%> (+0.04%)`	⬆️
aiohttp/web_urldispatcher.py	`99.51% <100%> (ø)`	⬆️
aiohttp/client_reqrep.py	`97.39% <100%> (+0.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93bad20...1e44acf. Read the comment docs.

pfreixes · 2018-01-26T08:51:47Z

aiohttp/client_reqrep.py

@@ -39,12 +39,18 @@
 __all__ = ('ClientRequest', 'ClientResponse', 'RequestInfo', 'Fingerprint')


-ContentDisposition = collections.namedtuple(
-    'ContentDisposition', ('type', 'parameters', 'filename'))
+@attr.s(frozen=True)


I have my doubts about the usage of frozen=True taking into account that immutability depends on the type of fields that you have in the data class. In the worst case scenario having a mutable attribute, the hash will fail and you won't prevent modify these attributes.

Sorry, I don't follow your point.

ConetentDisposition was immutable named tuple, I've just converted the class into immutable attrs representation. All type, parameters and filename attributes are immutable too.

... class Foo: ... a = attr.ib(type=MappingProxyType) ... >>> d = {} >>> m = MappingProxyType(d) >>> f = Foo(m) >>> hash(f) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "<attrs generated hash a57df858a086a8e0acf97d938d787d39b6f33d51>", line 4, in __hash__ TypeError: unhashable type: 'mappingproxy'```

pfreixes · 2018-01-26T10:25:28Z

aiohttp/helpers.py

+    type = attr.ib(type=str)
+    subtype = attr.ib(type=str)
+    suffix = attr.ib(type=str)
+    parameters = attr.ib(type=MultiDict)


MultiDict is mutable frozen=True is IMHO inconsistent in that case.

Agree, but afaik that mimics old behavior. May be worth to replace with frozen/proxy version?

kxepal

Few more notes, but nice improvement!

kxepal · 2018-01-26T10:41:11Z

aiohttp/web_request.py

-FileField = collections.namedtuple(
-    'Field', 'name filename file content_type headers')
+
+@attr.s(frozen=True, slots=True)


Rest namedtuple replaces comes without slots. Is that intentional?

Actually not . I forgot adding new param and was experimenting with different usage.

Let's not use slots in attrs, they are not bring new value but slightly slower.

kxepal · 2018-01-26T10:44:49Z

aiohttp/helpers.py

+    type = attr.ib(type=str)
+    subtype = attr.ib(type=str)
+    suffix = attr.ib(type=str)
+    parameters = attr.ib(type=MultiDict)


Agree, but afaik that mimics old behavior. May be worth to replace with frozen/proxy version?

kxepal · 2018-01-26T11:08:18Z

Btw, what do you think about dataclasses? Would we migrate to them eventually or will stay with attrs?

asvetlov · 2018-01-26T11:11:10Z

I want keeping frozen classes at least by default. It's safer.

webknjaz · 2018-01-26T11:11:49Z

By the way, attrs has a built-in @dataclass decorator

asvetlov · 2018-01-26T11:12:33Z

@kxepal we can return to discussion about switching to native python dataclasses after dropping Python 3.6 support. In several years.

asvetlov · 2018-01-26T11:13:34Z

@webknjaz I know about dataclass support embedded into attrs, but we cannot use it until dropping Python 3.5 support.

kxepal · 2018-01-26T11:14:01Z

@asvetlov
I mean...dataclasses are currently provided as standalone package, aren't they? We could just use them as optional dependency for <3.7.

asvetlov · 2018-01-26T11:15:16Z

https://pypi.python.org/pypi/dataclasses requires Python 3.6, we are limited to 3.5.3

kxepal · 2018-01-26T11:17:00Z

Yeah. Ok, I got your point. Thanks!

webknjaz · 2018-01-26T11:22:25Z

What if there was a backport?

asvetlov · 2018-01-26T12:09:02Z

@webknjaz don't follow

pfreixes · 2018-01-26T12:11:44Z

@asvetlov I don't see the point of usage by default frozen because its safer. The old namedtuples didn't give you that behavior. Start using the frozen=True means that you would give this feature for just for those params that are immutable. So, safer means that some attributes will be protected and others no? What I'm missing?

asvetlov · 2018-01-26T12:19:13Z

frozen prevents inplace modification:

In [1]: import attr

In [2]: @attr.s
   ...: class A:
   ...:     a = attr.ib()
   ...:     

In [3]: a = A(1)

In [4]: a.a
Out[4]: 1

In [5]: a.a = 2

In [6]: a.a
Out[6]: 2

In [7]: @attr.s(frozen=True)
   ...: class B:
   ...:     b = attr.ib()
   ...:     

In [8]: b = B(1)

In [9]: b.b
Out[9]: 1

In [10]: b.b = 2
---------------------------------------------------------------------------
FrozenInstanceError                       Traceback (most recent call last)
<ipython-input-10-dc6c31b33bcf> in <module>()
----> 1 b.b = 2

~/.virtualenvs/aiohttp/lib/python3.6/site-packages/attr/_make.py in _frozen_setattrs(self, name, value)
    349     Attached to frozen classes as __setattr__.
    350     """
--> 351     raise FrozenInstanceError()
    352 
    353 

FrozenInstanceError:

webknjaz · 2018-01-26T12:24:57Z

@asvetlov I mean, shouldn't it be possible to backport dataclasses implementation to support 3.5?

asvetlov · 2018-01-26T12:30:01Z

@webknjaz no, PEP 526 was implemented in Python 3.6 only.
Dataclasses require this feature.

pfreixes · 2018-01-26T12:34:00Z

this is clear, Im just saying that the usage of frozen can become error prone, this feature do not prevent manipulate mutable data, perhaps foo.headers.pop('header name'). IMHO this flag can give you a false sensation of protection. El 26/01/2018 13:19, "Andrew Svetlov" <notifications@github.com> escribió:

…

frozen prevents inplace modification: In [1]: import attr In [2]: @attr.s ...: class A: ...: a = attr.ib() ...: In [3]: a = A(1) In [4]: a.a Out[4]: 1 In [5]: a.a = 2 In [6]: a.a Out[6]: 2 In [7]: @attr.s(frozen=True) ...: class B: ...: b = attr.ib() ...: In [8]: b = B(1) In [9]: b.b Out[9]: 1 In [10]: b.b = 2 --------------------------------------------------------------------------- FrozenInstanceError Traceback (most recent call last) <ipython-input-10-dc6c31b33bcf> in <module>() ----> 1 b.b = 2 ~/.virtualenvs/aiohttp/lib/python3.6/site-packages/attr/_make.py in _frozen_setattrs(self, name, value) 349 Attached to frozen classes as __setattr__. 350 """ --> 351 raise FrozenInstanceError() 352 353 FrozenInstanceError: — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2690 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABK1iRPmY12oZP3ijVaFuR2xx_Ut5vOTks5tOcLCgaJpZM4RtmSA> .

webknjaz · 2018-01-26T12:39:06Z

Fair enough

asvetlov · 2018-01-26T12:50:28Z

@pfreixes I understand that frozen is not recursive property and all consequence pitfalls.
But frozen classes are better than non-frozen, at least in namedtuple replacement context.

pfreixes · 2018-01-26T13:24:17Z

Well, at least the whole replacement is safer for all variables, and must be the developer who has the last call to expose a variable mutable or not mutanle. A triki situatuion.

Anyway, lets move forward. LGTM

asvetlov · 2018-01-27T23:24:21Z

slots=True should be mandatory also -- without the flag attrs doesn't prevent setting a custom attribute, which is not desired behavior usually.

lock · 2019-10-28T10:04:24Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

asvetlov added 8 commits January 25, 2018 22:47

Convert FileField to attrs usage

6097f77

Convert RouteDef

ed0857d

Make RouteDef frozen

ed58ab0

Convert WebSocketReady to attrs

0f202bb

Convert more

7df77f9

Convert more

59a4a2d

Add changenote

edd88ec

Merge branch 'master' into attrs

880e806

asvetlov mentioned this pull request Jan 25, 2018

Change client tracing signatures #2686

Closed

pfreixes reviewed Jan 26, 2018

View reviewed changes

kxepal reviewed Jan 26, 2018

View reviewed changes

Drop slots=True

1e44acf

webknjaz approved these changes Jan 26, 2018

View reviewed changes

kxepal approved these changes Jan 26, 2018

View reviewed changes

asvetlov merged commit 50b81c5 into master Jan 26, 2018

asvetlov deleted the attrs branch January 27, 2018 23:23

asvetlov added a commit that referenced this pull request Jan 27, 2018

Fix #2690: Enforce slots for attrs classes

dd1986e

balloob mentioned this pull request Feb 7, 2018

Start using attrs instead of namedtuple home-assistant/architecture#4

Closed

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use attrs library for data classes #2690

Use attrs library for data classes #2690

asvetlov commented Jan 25, 2018

codecov-io commented Jan 25, 2018 •

edited

Loading

pfreixes Jan 26, 2018

asvetlov Jan 26, 2018

pfreixes Jan 26, 2018

pfreixes Jan 26, 2018

kxepal Jan 26, 2018

kxepal left a comment

kxepal Jan 26, 2018

asvetlov Jan 26, 2018

asvetlov Jan 26, 2018

kxepal Jan 26, 2018

kxepal commented Jan 26, 2018

asvetlov commented Jan 26, 2018

webknjaz commented Jan 26, 2018

asvetlov commented Jan 26, 2018

asvetlov commented Jan 26, 2018

kxepal commented Jan 26, 2018

asvetlov commented Jan 26, 2018

kxepal commented Jan 26, 2018

webknjaz commented Jan 26, 2018

asvetlov commented Jan 26, 2018

pfreixes commented Jan 26, 2018

asvetlov commented Jan 26, 2018

webknjaz commented Jan 26, 2018

asvetlov commented Jan 26, 2018

pfreixes commented Jan 26, 2018 via email

webknjaz commented Jan 26, 2018

asvetlov commented Jan 26, 2018

pfreixes commented Jan 26, 2018

asvetlov commented Jan 27, 2018

lock bot commented Oct 28, 2019

Use attrs library for data classes #2690

Use attrs library for data classes #2690

Conversation

asvetlov commented Jan 25, 2018

codecov-io commented Jan 25, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxepal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxepal commented Jan 26, 2018

asvetlov commented Jan 26, 2018

webknjaz commented Jan 26, 2018

asvetlov commented Jan 26, 2018

asvetlov commented Jan 26, 2018

kxepal commented Jan 26, 2018

asvetlov commented Jan 26, 2018

kxepal commented Jan 26, 2018

webknjaz commented Jan 26, 2018

asvetlov commented Jan 26, 2018

pfreixes commented Jan 26, 2018

asvetlov commented Jan 26, 2018

webknjaz commented Jan 26, 2018

asvetlov commented Jan 26, 2018

pfreixes commented Jan 26, 2018 via email

webknjaz commented Jan 26, 2018

asvetlov commented Jan 26, 2018

pfreixes commented Jan 26, 2018

asvetlov commented Jan 27, 2018

lock bot commented Oct 28, 2019

codecov-io commented Jan 25, 2018 •

edited

Loading