-
-
Notifications
You must be signed in to change notification settings - Fork 99
Fixed circular reference in HTTPConnection #755
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
base: main
Are you sure you want to change the base?
Fixed circular reference in HTTPConnection #755
Conversation
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #755 +/- ##
==========================================
- Coverage 84.01% 83.99% -0.03%
==========================================
Files 28 28
Lines 4235 4242 +7
==========================================
+ Hits 3558 3563 +5
- Misses 677 679 +2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this implementation adds complexity by putting cache management into multiple places. Can you find a decorator-based solution? It might be okay to add a dependency if there's a good implementation in the wild.
I'm not sure to understand: where is the complexity? A decorator-based implementation will result in the same issue than I really don't see the issue here. |
@francis-clairicia would using a |
@webknjaz This option seems good. The only drawback is that a If that's OK with you, then I can make the changes accordingly. |
@francis-clairicia I didn't realize it was writable. But here's how we can make it read-only. Put this helper into a >>> import functools as _ft
>>> class readonly_cached_property(_ft.cached_property):
... def __set__(self, instance, value):
... attr_name = self.attrname
... raise AttributeError(f'Attribute {attr_name} is read-only and cannot be set.')
>>> class AttrsCls:
... @cached_property
... def cp(self):
... return 42
... @readonly_cached_property
... def rocp(self):
... return 84
>>> attro = AttrsCls()
>>> attro.rocp
84
>>> attro.cp
42
>>> attro.rocp = 3
Traceback (most recent call last):
File "<python-input-42>", line 1, in <module>
attro.rocp = 3
^^^^^^^^^^
File "<python-input-37>", line 4, in __set__
raise AttributeError(f'Attribute {attr_name} is read-only and cannot be set.')
AttributeError: Attribute rocp is read-only and cannot be set. Additionally, make the change note a bit more specific. Referencing an abstract unnamed circular ref does not help a reader understand the context and the effect this has on them. |
β What kind of change does this PR introduce?
π What is the related issue number (starting with
#
)Resolves #746
β What is the current behavior? (You can also link to an open issue here)
The UNIX socket server's caching system for credentials involves a circular reference to
HTTPConnection
, even for an INET server.β What is the new behavior (if this is a feature change)?
The cache system is still in place, but now uses private attributes.
π Other information:
π Contribution checklist:
(If you're a first-timer, check out
this guide on making great pull requests)
the changes have been approved
and description in grammatically correct, complete sentences
This change isβ