gh-113317: Argument Clinic: tear out internal text accumulator APIs #113402
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
Conversation
…APIs Replace the internal accumulator APIs by using lists of strings and join(). Yak-shaving for separating out formatting code into a separate file.
Tools/clinic/clinic.py
Outdated
| """ | ||
| text, append, output = _text_accumulator() | ||
| return TextAccumulator(append, output) | ||
| TextAccumulator = list[str] |
There was a problem hiding this comment.
We could use this other places. For example in the c-render-data structure. But I'm not sure it adds much value.
Tools/clinic/clinic.py
Outdated
| """ | ||
| text, append, output = _text_accumulator() | ||
| return TextAccumulator(append, output) | ||
| TextAccumulator = list[str] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yeah, it makes sense to declare it just above BufferSeries.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Here's the PR: #113413
There was a problem hiding this comment.
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 :)
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
|
Thanks for the review and suggestions, Alex! |
…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 <Alex.Waygood@Gmail.com>
…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 <Alex.Waygood@Gmail.com>
…APIs (python#113402) Replace the internal accumulator APIs by using li 6DB6 sts of strings and join(). Yak-shaving for separating out formatting code into a separate file. Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
…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 <Alex.Waygood@Gmail.com>
Replace the internal accumulator APIs by using lists of strings and join().
Yak-shaving for separating out formatting code into a separate file.