GeezerVison

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

GeezerVison is showing github.com — gh-113317: Argument Clinic: tear out internal text accumulator APIs by erlend-aasland · Pull Request #113402 · python/cpython

Conversation

erlend-aasland commented Dec 22, 2023 •

edited by bedevere-app Bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

Replace the internal accumulator APIs by using lists of strings and join().

Yak-shaving for separating out formatting code into a separate file.

Issue: Argument Clinic: split out global stateless helpers and constants from clinic.py #113317

Sorry, something went wrong.

Uh oh!

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

All reactions


          pythongh-113317: Argument Clinic: tear out internal text accumulator …

cb85794

…APIs

Replace the internal accumulator APIs by using lists of strings and join().

Yak-shaving for separating out formatting code into a separate file.

erlend-aasland requested a review from AlexWaygood as a code owner

December 22, 2023 12:27

erlend-aasland added the skip news label

Dec 22, 2023

bedevere-app Bot added the awaiting core review label

Dec 22, 2023

bedevere-app Bot mentioned this pull request

Dec 22, 2023

Argument Clinic: split out global stateless helpers and constants from clinic.py #113317

Closed


          Add more suggestions

71534ea

erlend-aasland commented

Dec 22, 2023

View reviewed changes

Comment thread

Tools/clinic/clinic.py Outdated

		"""
		text, append, output = _text_accumulator()
		return TextAccumulator(append, output)
		TextAccumulator = list[str]

erlend-aasland Dec 22, 2023

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.

We could use this other places. For example in the c-render-data structure. But I'm not sure it adds much value.

Sorry, something went wrong.

Uh oh!

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

All reactions

erlend-aasland commented

Dec 22, 2023

View reviewed changes

Comment thread

Tools/clinic/clinic.py Resolved

Uh oh!

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


          Update Tools/clinic/clinic.py

71e0ead

AlexWaygood reviewed

Dec 22, 2023

View reviewed changes

Comment thread

Tools/clinic/clinic.py Outdated

		"""
		text, append, output = _text_accumulator()
		return TextAccumulator(append, output)
		TextAccumulator = list[str]

AlexWaygood Dec 22, 2023

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.

Nit: this type alias isn't actually used until much further down in the file. Could we maybe move it to line 2345 or something?

Sorry, something went wrong.

Uh oh!

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

All reactions

AlexWaygood Dec 22, 2023 •

edited

Loading

Uh oh!

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

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.

Or we can just get rid of it, like you propose in #113402 (comment)! I'm easy either way. Getting rid of it might be best for now, it isn't immediately obvious what a "TextAccumulator" means in terms of the types involved

Sorry, something went wrong.

Uh oh!

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

All reactions

erlend-aasland Dec 22, 2023

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.

Yeah, it makes sense to declare it just above BufferSeries.

Sorry, something went wrong.

Uh oh!

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

All reactions

erlend-aasland Dec 22, 2023

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.

Done

Sorry, something went wrong.

Uh oh!

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

All reactions

erlend-aasland Dec 22, 2023

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.

I initially kept it, just to keep the diff down, but I'm fine with letting it go. I prefer deal with it in a follow-up PR, though.

Sorry, something went wrong.

Uh oh!

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

AlexWaygood reacted with thumbs up emoji

All reactions

👍 1 reaction

erlend-aasland Dec 22, 2023

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.

Here's the PR: #113413

Sorry, something went wrong.

Uh oh!

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

AlexWaygood reacted with heart emoji

All reactions

❤️ 1 reaction


          Move TextAccumulator

b1ff628

AlexWaygood approved these changes

Dec 22, 2023

View reviewed changes

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

This is great -- a big readability improvement, imo. And much less code for us to maintain!

We could use this other places. For example in the c-render-data structure. But I'm not sure it adds much value.

We posted comments simultaneously, I think -- I'd be happy to see the TextAccumulator type alias gone, as I mentioned in #113402 (comment)! But I'm also fine with it staying, if you prefer it :)

Sorry, something went wrong.

Uh oh!

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

erlend-aasland reacted with hooray emoji

erlend-aasland reacted with heart emoji

All reactions

🎉 1 reaction
❤️ 1 reaction

bedevere-app Bot added awaiting merge and removed awaiting core review labels

Dec 22, 2023

AlexWaygood reviewed

Dec 22, 2023

View reviewed changes

Comment thread

Tools/clinic/clinic.py Resolved

Uh oh!

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

Comment thread

Tools/clinic/clinic.py Resolved

Uh oh!

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


          Update clinic.py

144b115

Co-authored-by: Alex Waygood <[email protected]>

erlend-aasland merged commit daa658a into python:main

Dec 22, 2023

erlend-aasland deleted the clinic/remove-accumulator branch

December 22, 2023 20:28

bedevere-app Bot removed the awaiting merge label

Dec 22, 2023

erlend-aasland commented Dec 22, 2023

Copy link

Copy Markdown

Contributor Author

Thanks for the review and suggestions, Alex!

AlexWaygood reacted with rocket emoji

All reactions

🚀 1 reaction

Sorry, something went wrong.

Uh oh!

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

erlend-aasland mentioned this pull request

Dec 22, 2023

gh-113317: Remove TextAccumulator type alias from clinic.py #113413

Merged

erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request

Dec 22, 2023


          pythongh-113317: Remove TextAccumulator type alias from clinic.py

871ee2d

Clean-up after pythongh-113402.

erlend-aasland added a commit that referenced this pull request

Dec 22, 2023


          gh-113317: Remove TextAccumulator type alias from clinic.py (#113413)

9c3ddf3

Clean-up after gh-113402.

ryan-duve pushed a commit to ryan-duve/cpython that referenced this pull request

Dec 26, 2023


          pythongh-113317: Argument Clinic: tear out internal text accumulator …

07e2364

…APIs (python#113402)

Replace the internal accumulator APIs by using lists of strings and join().

Yak-shaving for separating out formatting code into a separate file.

Co-authored-by: Alex Waygood <[email protected]>

ryan-duve pushed a commit to ryan-duve/cpython that referenced this pull request

Dec 26, 2023


          pythongh-113317: Remove TextAccumulator type alias from clinic.py (py…

6c3748c

…thon#113413)

Clean-up after pythongh-113402.

kulikjak pushed a commit to kulikjak/cpython that referenced this pull request

Jan 22, 2024


          pythongh-113317: Argument Clinic: tear out internal text accumulator …

ca42f24

…APIs (python#113402)

Replace the internal accumulator APIs by using lists of strings and join().

Yak-shaving for separating out formatting code into a separate file.

Co-authored-by: Alex Waygood <[email protected]>

kulikjak pushed a commit to kulikjak/cpython that referenced this pull request

Jan 22, 2024


          pythongh-113317: Remove TextAccumulator type alias from clinic.py (py…

a641859

…thon#113413)

Clean-up after pythongh-113402.

aisk pushed a commit to aisk/cpython that referenced this pull request

Feb 11, 2024


          pythongh-113317: Argument Clinic: tear out internal text accumulator …

a1db063

…APIs (python#113402)

Replace the internal accumulator APIs by using lists of strings and join().

Yak-shaving for separating out formatting code into a separate file.

Co-authored-by: Alex Waygood <[email protected]>

aisk pushed a commit to aisk/cpython that referenced this pull request

Feb 11, 2024


          pythongh-113317: Remove TextAccumulator type alias from clinic.py (py…

059d73d

…thon#113413)

Clean-up after pythongh-113402.

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request

Sep 2, 2024


          pythongh-113317: Argument Clinic: tear out internal text accumulator …

953e82d

…APIs (python#113402)

Replace the internal accumulator APIs by using lists of strings and join().

Yak-shaving for separating out formatting code into a separate file.

Co-authored-by: Alex Waygood <[email protected]>

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request

Sep 2, 2024


          pythongh-113317: Remove TextAccumulator type alias from clinic.py (py…

b9e3ddb

…thon#113413)

Clean-up after pythongh-113402.

Reviewers

AlexWaygood

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

2 participants

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 01402520 — 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.

GeezerVison

Conversation

erlend-aasland commented Dec 22, 2023 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexWaygood Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erlend-aasland Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

erlend-aasland Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

erlend-aasland Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

erlend-aasland Dec 22, 2023

Choose a reason for hiding this comment

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erlend-aasland commented Dec 22, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

erlend-aasland commented Dec 22, 2023 •

edited by bedevere-app Bot

Loading

Uh oh!

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

AlexWaygood Dec 22, 2023 •

edited

Loading

Uh oh!

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