GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-46752: Uniform TaskGroup.__repr__ by asvetlov · Pull Request #31409 · python/cpython

Conversation

@asvetlov

@asvetlov asvetlov commented Feb 18, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

https://bugs.python.org/issue46752

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@asvetlov
Uniform TaskGroup.__repr__
a2e51ab
@asvetlov
asvetlov requested a review from 1st1 as a code owner February 18, 2022 16:09
@bedevere-bot bedevere-bot added the awaiting core review label Feb 18, 2022
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 18, 2022
@asvetlov
asvetlov requested a review from gvanrossum February 18, 2022 16:09
@asvetlov asvetlov added the skip news label Feb 18, 2022
gvanrossum
gvanrossum reviewed Feb 18, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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.

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.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/asyncio/taskgroups.py Outdated
Comment on lines +41 to +42
info_str = ' ' + ' '.join(info)
return f'<TaskGroup{info_str}>'

@gvanrossum gvanrossum Feb 18, 2022

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@asvetlov asvetlov Feb 18, 2022 •
edited by gvanrossum
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@1st1 1st1 Feb 18, 2022

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Feb 18, 2022

Copy link
Copy Markdown
Member

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Feb 18, 2022

Copy link
Copy Markdown
Member

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.

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@asvetlov asvetlov Feb 18, 2022

Copy link
Copy Markdown
Contributor Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
1st1 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@asvetlov
Replace : with =
0248f31
@asvetlov

asvetlov commented Feb 18, 2022

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@asvetlov
Init repr's info as ['']
eee2cc9
@asvetlov

asvetlov commented Feb 18, 2022

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
1st1
1st1 approved these changes Feb 18, 2022
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Feb 18, 2022
gvanrossum
gvanrossum approved these changes Feb 19, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@asvetlov
asvetlov merged commit e7130c2 into main Feb 20, 2022
@asvetlov
asvetlov deleted the taskgroup-repr branch February 20, 2022 10:07
@bedevere-bot bedevere-bot removed the awaiting merge label Feb 20, 2022
@gvanrossum gvanrossum mentioned this pull request Apr 27, 2022
Introduce task groups to asyncio and change task cancellation semantics #90908
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@1st1 1st1 1st1 approved these changes
@gvanrossum gvanrossum gvanrossum approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@asvetlov @1st1 @gvanrossum @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.