Add slots and -> None to __init__ in classes

[CoolCat467]

This PR adds slots to classes and missing return None type annotations to __init__ functions in _dtls.py.

[codecov]

Codecov Report

Merging #2723 (aa2b4a8) into master (59229b1) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head aa2b4a8 differs from pull request most recent head 97d4d0a. Consider uploading reports for the commit 97d4d0a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2723      +/-   ##
==========================================
- Coverage   98.90%   98.87%   -0.04%     
==========================================
  Files         113      113              
  Lines       16685    16605      -80     
  Branches     3025     3014      -11     
==========================================
- Hits        16503    16418      -85     
- Misses        125      129       +4     
- Partials       57       58       +1     

Files Changed	Coverage Δ
trio/_dtls.py	96.62% <100.00%> (+0.03%)	⬆️

... and 15 files with indirect coverage changes

[TeamSpen210]

Slots definitely would be a slight improvement in a lot of cases, but we might need to be careful - they technically are backwards incompatible, since arbitrary attributes can't be added anymore.

[A5rocks]

IMO we should only add -> None on __init__s that need it (ie where there are no other typed parameters). But this is just a styling thing which isn't too big a deal.

[jakkdl]

IMO we should only add -> None on __init__s that need it (ie where there are no other typed parameters). But this is just a styling thing which isn't too big a deal.

yeah no_untyped_defs will catch __init__s that require it, so unless we configure mypy (if that's possible? I tried looking for the config option for that) to always require it I don't think PRs should go out of their way to add them.

[jakkdl]

I could send you my (messy and inofficial) tox.ini if you want

[jakkdl]

What's the case for adding a static __slots__? If it's Good:tm: should they be added everywhere? (where attr doesn't already generate them)

[TeamSpen210]

Well the main purpose is that it improves memory usage, since the __dict__ for attributes don't need to be created. And as a side effect which can also be useful, additional attribute names can't be used. Though, in 3.11+ optimisations to regular objects means that the memory improvement is basically negligible. The other issue is that they break weakrefs unless you add a slot for that (which is why the tests are all failing).

[jakkdl]

Well the main purpose is that it improves memory usage, since the __dict__ for attributes don't need to be created. And as a side effect which can also be useful, additional attribute names can't be used. Though, in 3.11+ optimisations to regular objects means that the memory improvement is basically negligible. The other issue is that they break weakrefs unless you add a slot for that (which is why the tests are all failing).

that sounds ... not important enough to bother then imho?

[A5rocks]

Apologies, I don't think this PR is worth merging. The benefits of slots on most classes don't outweigh the downsides (IMO). We've added them to most of the pertinent classes, too. The -> None is unnecessary and I personally don't like inconsistency like that: we won't get told if we're ever missing it.

(I'm really on the edge about the slots thing, though...)

What do you think? Also I'm in no way saying this PR will not get merged, just offering my take!

[CoolCat467]

Personally I always make sure to include slots in all the classes I can, because first off when you use help() on classes with slots, it tells you about all the non-private variables without having to dig through the docs, and additionally because it allows users down the line to use slots effectively themselves in sub-classes.

[TeamSpen210]

For help(), really we should just add the attributes to the docstring, and/or add annotations to declare them for the class. __slots__ just tells you the names, not what they do so it's kinda limited. Most of Trio's classes can't be subclassed, so that doesn't exactly apply. I don't really think this is necessary, it would only help on classes that are being spawned heaps of times. A lot of those use attrs anyway which does this automatically.

The big issue is backwards compatibility - any user assigning custom attributes or using weakrefs is going to be broken by this. You can manually add the __weakref__ slot back, but then the memory saving is effectively zeroed out.

[jakkdl]

Closing this, adding __slots__ does not seem to have clear benefits and been voted down, and -> None on __init__s with typed arguments doesn't do anything.