Skip to content
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

Explain use of global thread-local variable in repr. #854

Closed
wants to merge 1 commit into from

Conversation

thetorpedodog
Copy link
Contributor

It's not completely obvious to a reader why we need a thread-local
"global" variable to track reference cycles in __repr__ calls,
and the test does not currently contain one. This change adds a comment
explaining the need and also adds a non-attrs value in the reference
cycle of test_infinite_recursion.


This was inspired by a bug that was opened where the reporter couldn't see why the global variable was needed in the __repr__ process. Initially I couldn't either, so I figured it was a good idea to add an explanation of why it is there—turns out it is an excellent solution to a real problem. I will gladly accept input about the nature/phrasing/placement of the commentary.

This is essentially a docs-only change (with a minor test change attached) so I don't think the full checklist applies in this case.

@hynek
Copy link
Member

hynek commented Oct 31, 2021

Can you please fix the line lengths? See https://github.com/python-attrs/attrs/runs/4056466583?check_suite_focus=true#step:5:43

I wonder if we should maybe add a blurb to the repr argument to explain, that setting it True involves threadlocals?

@thetorpedodog
Copy link
Contributor Author

Can you please fix the line lengths?

79 characters! My worst enemy!

I wonder if we should maybe add a blurb to the repr argument to explain, that setting it True involves threadlocals?

I actually have an idea that may solve the cloudpickle incompatibility (and doesn’t involve any special handling on either side), which I am going to try once I get into work tomorrow. If that does the trick, I can create a new change with the fix, but if not I’ll add that note to the docs.

@thetorpedodog
Copy link
Contributor Author

I actually have an idea that may solve the cloudpickle incompatibility (and doesn’t involve any special handling on either side), which I am going to try once I get into work tomorrow. If that does the trick, I can create a new change with the fix, but if not I’ll add that note to the docs.

Update: #857

It's not completely obvious to a reader why we need a thread-local
"global" variable to track reference cycles in `__repr__` calls,
and the test does not currently contain one. This change adds a comment
explaining the need and also adds a non-attrs value in the reference
cycle of `test_infinite_recursion`.
@hynek
Copy link
Member

hynek commented Nov 4, 2021

Closing thanks to #857!

@hynek hynek closed this Nov 4, 2021
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.

2 participants