Skip to content

bpo-46752: Uniform TaskGroup.__repr__ #31409

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

Merged
merged 3 commits into from
Feb 20, 2022
Merged

bpo-46752: Uniform TaskGroup.__repr__ #31409

merged 3 commits into from
Feb 20, 2022

Conversation

asvetlov
Copy link
Contributor

@asvetlov asvetlov commented Feb 18, 2022

Other asyncio objects use prop=val format, not prop:val.

https://bugs.python.org/issue46752

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't you say other asyncio objects use prop=val? Were you going to add that? (I guess it'll require updating a few tests.)

Comment on lines 41 to 42
info_str = ' ' + ' '.join(info)
return f'<TaskGroup{info_str}>'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
info_str = ' ' + ' '.join(info)
return f'<TaskGroup{info_str}>'
info_str = ' '.join(info)
return f'<TaskGroup {info_str}>'

Copy link
Contributor Author

@asvetlov asvetlov Feb 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After applying the suggestion, a repr for created TaskGroup before __enter__() is called will be "<TaskGroup >" (note the space after class name).
Is it ok? Should we care about this rare case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO accumulating strings in [] isn't a great pattern unless you have to use it for performance reasons. /my2c.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After applying the suggestion, a repr for created TaskGroup before __enter__() is called will be "<TaskGroup >" (note the space after class name). Is it ok? Should we care about this rare case?

The original code has the same issue AFAICT. If you don't want that you could initialize info = [''] perhaps?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO accumulating strings in [] isn't a great pattern unless you have to use it for performance reasons. /my2c.

Meh. IIRC the info pattern is used all over asyncio (e.g. _task_repr_info in base_tasks.py).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

info = [''] is clever, applied.

Yes, info list is used by other asyncio repr's. I'd like to have the similar approach across the library.

@asvetlov
Copy link
Contributor Author

Oops, I've mentioned the problem ': vs =' problem but didn't actually resolve it.
Fixed. I'm sorry. Now = is used.

@asvetlov
Copy link
Contributor Author

@gvanrossum do you want tests for __repr__?
I'm lazy but can write them on demand.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for tests. Maybe a new contributor will add them. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants