Skip to content

Add slots to exceptions#2737

Closed
CoolCat467 wants to merge 4 commits intopython-trio:masterfrom
CoolCat467:exceptions-slots
Closed

Add slots to exceptions#2737
CoolCat467 wants to merge 4 commits intopython-trio:masterfrom
CoolCat467:exceptions-slots

Conversation

@CoolCat467
Copy link
Member

This PR adds slots to all the exceptions defined in _core/_exceptions.py

@codecov
Copy link

codecov bot commented Aug 5, 2023

Codecov Report

Merging #2737 (57caad2) into master (722f1b5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2737   +/-   ##
=======================================
  Coverage   98.90%   98.90%           
=======================================
  Files         113      113           
  Lines       16686    16694    +8     
  Branches     3025     3025           
=======================================
+ Hits        16504    16512    +8     
  Misses        125      125           
  Partials       57       57           
Files Changed Coverage Δ
trio/_core/_exceptions.py 100.00% <100.00%> (ø)

@jakkdl
Copy link
Member

jakkdl commented Aug 6, 2023

Is there a stronger case for this than #2723?

@CoolCat467
Copy link
Member Author

In this one, nothing should be weak referencing exceptions

@jakkdl
Copy link
Member

jakkdl commented Aug 8, 2023

but is there much/anything to gain? There's a small cost in code readability and there's no CI check that makes usage consistent or anything

@CoolCat467
Copy link
Member Author

I feel like it's best practice to use slots whenever possible, and more generally (not applicable here as much though) having slots on your classes means if people want to subclass them they can also take advantage of lower memory usage per object if they so choose. Not having slots eliminates that option for people down the road.

@A5rocks
Copy link
Contributor

A5rocks commented Aug 10, 2023

I'm very ambivalent about this one. It's probably fine! But I don't feel very strongly about specifically adding __slots__ to exceptions either way. Maybe someone else with strong(ish) opinions one way or another can figure this out...?

@oremanj
Copy link
Member

oremanj commented Aug 10, 2023

A BaseException already has a __dict__, so the only effect of adding __slots__ = () is to disable weak references. Which only saves eight bytes per exception object, and could plausibly break something even though I agree it's unlikely.

I don't think this is worth the trouble.

@TeamSpen210
Copy link
Contributor

Also exception objects aren't exactly created / kept around long, so it's not really that big a deal how big they are.

@jakkdl
Copy link
Member

jakkdl commented Aug 11, 2023

I'll close this one for now then

@jakkdl jakkdl closed this Aug 11, 2023
@CoolCat467 CoolCat467 deleted the exceptions-slots branch August 24, 2023 01:56
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.

5 participants