gh-113317: Argument Clinic: tear out internal text accumulator APIs #113402
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 <[email protected]>
|
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 <[email protected]>
…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]>
…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]>
…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]>
Replace the internal accumulator APIs by using lists of strings and join().
Yak-shaving for separating out formatting code into a separate file.