bpo-44544: [doc] list all textwrap func kwargs by jdevries3133 · Pull Request #26999 · python/cpython

jdevries3133 · 2021-07-02T17:57:00Z

list all kwargs for textwrap.wrap, textwrap.fill, and
textwrap.shorten
now, there are nav links to attributes of TextWrap, which makes navigation
much easier while minimizing duplication
revised descriptions of those functions accordingly
did not change actual function signatures; maybe we should?
- if so, we should continue accepting additional kwargs as to not break
  existing code.
- now, function signatures do not match the documentation
- however, I noticed that while using pyright, it still shows all the kwargs
  through introspection. Maybe the mismatch is acceptable?
https://bugs.python.org/issue44544

* list all kwargs for `textwrap.wrap`, `textwrap.fill`, and `textwrap.shorten` * now, there are nav links to attributes of TextWrap, which makes navigation much easier while minimizing duplication

akulakov

Thanks! Left a few comments, nothing is wrong per se, just making it more consize..

Doc/library/textwrap.rst

jdevries3133 · 2021-07-02T19:46:21Z

@akulakov Thank you for your review!

akulakov · 2021-07-02T20:36:32Z

I should have mentioned I understand the attractiveness of links.. I’m a bit on the fence on this because of it being duplicated 3 times and already having a link to textwrapper... let me think about it a bit.

…

On Fri, Jul 2, 2021 at 3:38 PM Jack DeVries ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Doc/library/textwrap.rst <#26999 (comment)>: > - See the :meth:`TextWrapper.wrap` method for additional details on how - :func:`wrap` behaves. + * :attr:`.initial_indent` The nice thing about that listing, though, is that it forms hyperlinks directly to each of these attributes, so it makes html browsing very smooth (see screenshot) [image: Screen Shot 2021-07-02 at 3 38 15 PM] <https://user-images.githubusercontent.com/58614260/124321338-86786780-db4b-11eb-938b-0e044eccea47.png> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#26999 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB3RITZHXANEFBG6KAHD6TTVYIUTANCNFSM47XEJQUQ> .

Doc/library/textwrap.rst

serhiy-storchaka · 2021-07-02T20:42:48Z

Doc/library/textwrap.rst

+                   fix_sentence_endings=False, \
+                   break_long_words=True, drop_whitespace=True, \
+                   break_on_hyphens=True, tabsize=8, \
+                   *, max_lines=None, **kwargs)


Where is placeholder? What does kwargs include?

I thought that placeholder only has an effect in :func:shorten?

**kwargs is there just because the actual function signature is like this:

def wrap(text, width=70, **kwargs): ...

Any keyword argument will be accepted, but only correctly spelled kwargs will have an effect. I added an additional note to more specifically highlight this behavior; it adds on to the paragraphs where these kwargs are listed:

Be careful not to misspell keyword arguments. Misspelled keyword arguments will silently have no effect, and an
exception will not be raised.

I don't think that's the case, the kwargs are passed to TextWrapper, which will not accept unknown keyword args:

/usr/local/Cellar/python@3.9/3.9.1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/textwrap.py in shorten(text, width, **kwargs) 403 'Hello [...]' 404 """ --> 405 w = TextWrapper(width=width, max_lines=1, **kwargs) 406 return w.fill(' '.join(text.strip().split())) 407 TypeError: __init__() got an unexpected keyword argument 'zz'

placeholder can apply to all 3 functions because it depends on max_lines -- if max_lines is specified and results in removing of some lines, placeholder will be added per docs.

jdevries3133 · 2021-07-02T20:43:13Z

If anything, I'd say maybe change the function signature back to what it originally (.. function:: wrap(text, width=70, **kwargs)), and keep the links. With that many arguments, the function signature kind of looks like spaghetti and I think that the browsing experience is much stronger with the links.

akulakov · 2021-07-02T20:51:54Z

The reason I didn’t like having just **kwargs in the sig is that it is inconsistent and confusing, as I argued in the ticket, so whether the links are kept or not, I think it’s preferable to keep the long signatures.. yes hard to read but if we have function with many args, that’s just what you see is what you get :)

…

On Fri, Jul 2, 2021 at 4:43 PM Jack DeVries ***@***.***> wrote: If anything, I'd say maybe change the function signature back to what it originally (.. function:: wrap(text, width=70, **kwargs)), and keep the links. With that many arguments, the function signature kind of looks like spaghetti and I think that the browsing experience is much stronger with the links. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#26999 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB3RIRX7BYOO6JO23SN5ADTVYQGZANCNFSM47XEJQUQ> .

akulakov · 2021-07-03T15:34:18Z

A couple of thoughts:

I haven't seen instances in Python docs where a list of the same links would be duplicated multiple times closely, so I feel like that doesn't match this pattern of avoiding too much repetition.

An option that "splits the difference" would be to have the list of links for the first function and omit lists for the other two. I think either this option or not having links at all would work well.

akulakov · 2021-07-07T18:45:56Z

I saw a very similar example in current docs: https://docs.python.org/3.11/library/logging.html#logging.LoggerAdapter -- the methods are from a different class but are linked here, in one paragraph rather than separate lines. Seems like a good approach, WDYT?

jdevries3133 · 2021-07-07T20:09:48Z

@akulakov that seems like a good precedent to follow. See 1306f2d; what do you think?

akulakov · 2021-07-07T20:28:44Z

@akulakov that seems like a good precedent to follow. See 1306f2d; what do you think?

Looks good, thanks!

merwok · 2021-07-09T18:54:14Z

The few lines with inline links in the logging example look better than the list of raw names in the screenshot here, though.

akulakov · 2021-07-09T19:08:19Z

@merwok yep, the screenshot is outdated. We went with the list of inline links.

@jdevries3133 btw Serhiy also left a few requests for change above (not sure if you saw them).

iritkatriel · 2021-07-27T13:17:14Z

Doc/library/textwrap.rst

+   :attr:`.drop_whitespace`, :attr:`.break_on_hyphens` :attr:`.tabsize`, and
+   :attr:`.max_lines`.  Be careful not to misspell keyword arguments.
+   Misspelled keyword arguments will silently have no effect, and an exception
+   will not be raised.


In my view repetition in the docs is not better than repetition in code.

The root cause of the repetition is that these functions all recieve a laundry list of keyword arguments, and they are all the same or highly similar. In that sense, the API is repetitive. Maybe it could have been designed better if we had dataclasses for grouping the values passed in or something like that, but I think the repetition in the documentation is a consequence of the library'a design, and we aren't going to change the API design.

In the code, this list of args appears once in the constructor of TextWrapper. The module-level wrapper functions just forward their kwargs to this constructor. It would be enough to define all these args once and in other places say "the meaning of the parameters are the same as over there".

It's easy for these repetitions to go out of sync with each other (if you add an arg to TextWrapper.init you need to update the doc in multiple places).

I don't see what the benefit of the repetition is. For a user trying to study the library for the first time it's just tedious. For someone looking up a single function, clicking on a link to the TextWrapper constructor is not complicated.

My preference would also be to avoid repeated list of links. I previously suggested inline links as a compromise between no links and the "tall" list with a link per line as in initial revision. I don't have a strong opinion though, as long as it's not a "tall" list.

@iritkatriel I'm sorry, I thought you were talking about repetition between the function signature and the inline links but now I realize I don't think that's what you're talking about. I'm going to take a closer look later today, sorry for any confusion.

Ah, sorry I was vague.

@iritkatriel I see what you are saying. I went ahead and removed the repetitive links all together. Do you think that the explicit function signatures is a good change? Otherwise, there's nothing to be done here.

Good question, I wonder if the documentation working group can come up with suggestions. This is not the only library that has this pattern. @JulienPalard @Mariatta

akulakov · 2021-07-28T12:14:35Z

Doc/library/textwrap.rst

+   arguments. Note that the whitespace is collapsed before the text is passed
+   to the :class:`TextWrapper` :meth:`fill` function, so changing the value of
+   :attr:`.tabsize`, :attr:`.expand_tabs`, :attr:`.drop_whitespace`, and
+   :attr:`.replace_whitespace` will have no effect.  Be careful not to misspell


The last sentence can be omitted because the same can be said for any func / method in stdlib.

Doc/library/textwrap.rst

akulakov · 2021-07-28T12:38:51Z

One interesting question that I've just thought about is whether **kwargs should be included in the sigs of 3 functions. The sigs don't follow the code exactly, that's the point of this PR. The users are meant to follow the signature and use the args listed there. **kwargs is not to be used by them.

So the argument is that having **kwargs in the sig will probably only add confusion; the only utility may conceivably be that user is alerted to the idea that if they accidentally use an arg from a different function (out of these 3), it will not raise a TypeError.

So it seems **kwargs is just a remnant from the old, intentionally empty sig. Thoughts?

merwok · 2021-07-28T13:53:13Z

Doc/library/textwrap.rst

   Wraps the single paragraph in *text* (a string) so every line is at most
   *width* characters long.  Returns a list of output lines, without final
-   newlines.
+   newlines.  *width* defaults to ``70``.


Why duplicate info that is already visible in the signature?

This is just carried over from the current doc, and not relevant to the bpo, so I left it:

https://docs.python.org/3/library/textwrap.html#textwrap.wrap

Oh, my bad, in the old documentation, that sentence was in the docs for textwrap.wrap, and for some reason I though it'd be a good idea to bring it down here so that they match, but that's definitely not right! If anything, it'd be better to remove the unnecessary repetition from textwrap.wrap, but again, that's out of the scope of this bpo, and we can make a new little bpo for that problem after this one lands.

Either way, I'm working on reverting all changes to things other than the function signatures right now, because I think that's all we really want to do here.

merwok · 2021-07-28T13:57:43Z

The users are meant to follow the signature and use the args listed there. **kwargs is not to be used by them.

I don’t really understand what you mean here, maybe because **kwargs means two different things in function definitions and function calls.
The textwrap functions are defined with varkwargs, but not any param is accepted, only ones valid for TextWrap.
As for calling, users can certainly use kwargs unpacking, if they have a dict of valid params on hand.

akulakov · 2021-07-28T14:21:55Z

The users are meant to follow the signature and use the args listed there. **kwargs is not to be used by them.

I don’t really understand what you mean here, maybe because **kwargs means two different things in function definitions and function calls.
The textwrap functions are defined with varkwargs, but not any param is accepted, only ones valid for TextWrap.
As for calling, users can certainly use kwargs unpacking, if they have a dict of valid params on hand.

I meant to say that in this particular case, **kwargs would imply to a user that they can use some other kwargs in addition to ones explicitly listed. But if they did that, they would either get a TypeError from TextWrapper, or in case of one of the functions, the kwargs they might use would be a no-op, and so should not be used.

jdevries3133 · 2021-07-28T14:58:26Z

@akulakov @merwok @serhiy-storchaka @iritkatriel I went ahead and reverted any changes other than those to the documented function signature. I think that changes to the function signature are the closest to the bpo, and make the most sense to add. I think that Irit made good points about the redundant links introducing a maintainability problem and not providing a significant enough usability improvement.

Andrei, I agree that documenting **kwargs is not necessary. The kwargs are always forwarded immediately to the TextWrap constructor, which does not accept arbitrary arguments:

def __init__(self,
             width=70,
             initial_indent="",
             subsequent_indent="",
             expand_tabs=True,
             replace_whitespace=True,
             fix_sentence_endings=False,
             break_long_words=True,
             drop_whitespace=True,
             break_on_hyphens=True,
             tabsize=8,
             *,
             max_lines=None,
             placeholder=' [...]'):

So, you get what you would expect from a misspelled keyword argument:

>>> import textwrap
>>> textwrap.wrap('hi', widt=10)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/johndevries/.pyenv/versions/3.9.6/lib/python3.9/textwrap.py", line
378, in wrap
    w = TextWrapper(width=width, **kwargs)
TypeError: __init__() got an unexpected keyword argument 'widt'

I had this wrong in earlier revisions of this PR, but I (mostly) fixed it in 2927e0d; "fix: incorrect information; TextWrapper.init will raise Exc," and later removed all non-function-signature changes as I mentioned above.

Summary

I reverted all changes other than those to documented function signatures
Documented function signatures do not include **kwargs; rationale above.
I agree with Éric that the repetitive phrase, `width defaults to ``70``` currently on line 31 should be removed, but it's out of the scope of this bpo. See bpo-44763: [doc] remove repetitive sentence from textwrap.wrap #27423 and bpo-44763.

miss-islington · 2021-07-28T15:14:57Z

Thanks @jdevries3133 for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.9.
🐍🍒⛏🤖

bedevere-bot · 2021-07-28T15:15:06Z

GH-27424 is a backport of this pull request to the 3.10 branch.

(cherry picked from commit c1e39d6) Co-authored-by: Jack DeVries <58614260+jdevries3133@users.noreply.github.com>

bedevere-bot · 2021-07-28T15:15:11Z

GH-27425 is a backport of this pull request to the 3.9 branch.

(cherry picked from commit c1e39d6) Co-authored-by: Jack DeVries <58614260+jdevries3133@users.noreply.github.com>

bpo-44544: [doc] list all textwrap func kwargs

91b9e7d

* list all kwargs for `textwrap.wrap`, `textwrap.fill`, and `textwrap.shorten` * now, there are nav links to attributes of TextWrap, which makes navigation much easier while minimizing duplication

the-knights-who-say-ni added the CLA signed label Jul 2, 2021

bedevere-bot added awaiting review docs Documentation in the Doc dir labels Jul 2, 2021

fix: add blurb

e54d50a

akulakov requested changes Jul 2, 2021

View reviewed changes

Doc/library/textwrap.rst Outdated Show resolved Hide resolved

Doc/library/textwrap.rst Outdated Show resolved Hide resolved

Doc/library/textwrap.rst Outdated Show resolved Hide resolved

Doc/library/textwrap.rst Outdated Show resolved Hide resolved

bedevere-bot added awaiting core review and removed awaiting review labels Jul 2, 2021

jdevries3133 added 3 commits July 2, 2021 15:39

remove note about same kwargs

13e025a

add **kwargs to func signatures

aa2ec6a

restore previous caveat paragraph for textwrap.shorten

c346c1d

serhiy-storchaka reviewed Jul 2, 2021

View reviewed changes

cleanup repetitive kwarg listings

1306f2d

bpo-44544: respond to review from @@serhiy-storchaka

d6b5fe0

iritkatriel reviewed Jul 27, 2021

View reviewed changes

jdevries3133 added 3 commits July 28, 2021 00:01

revert accidental formatting change

ec96929

remove repetitive keyword argument links

cd5224c

fix: incorrect information; TextWrapper.__init__ will raise Exc

2927e0d

akulakov reviewed Jul 28, 2021

View reviewed changes

Doc/library/textwrap.rst Show resolved Hide resolved

akulakov reviewed Jul 28, 2021

View reviewed changes

Doc/library/textwrap.rst Outdated Show resolved Hide resolved

merwok reviewed Jul 28, 2021

View reviewed changes

fix extra * in function signatures, and remove **kwargs

1315486

revert all changes in textwrap.rst not related to func signatures

9be6149

ambv added needs backport to 3.9 needs backport to 3.10 only security fixes labels Jul 28, 2021

ambv merged commit c1e39d6 into python:main Jul 28, 2021

bedevere-bot removed awaiting core review needs backport to 3.10 only security fixes labels Jul 28, 2021

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 28, 2021

bpo-44544: [doc] list all textwrap func kwargs (pythonGH-26999)

eb81d17

(cherry picked from commit c1e39d6) Co-authored-by: Jack DeVries <58614260+jdevries3133@users.noreply.github.com>

bedevere-bot removed the needs backport to 3.9 label Jul 28, 2021

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 28, 2021

bpo-44544: [doc] list all textwrap func kwargs (pythonGH-26999)

4c4287a

(cherry picked from commit c1e39d6) Co-authored-by: Jack DeVries <58614260+jdevries3133@users.noreply.github.com>

ambv pushed a commit that referenced this pull request Jul 28, 2021

bpo-44544: [doc] list all textwrap func kwargs (GH-26999) (GH-27424)

97af790

(cherry picked from commit c1e39d6) Co-authored-by: Jack DeVries <58614260+jdevries3133@users.noreply.github.com>

ambv pushed a commit that referenced this pull request Jul 28, 2021

bpo-44544: [doc] list all textwrap func kwargs (GH-26999) (GH-27425)

019a8b0

(cherry picked from commit c1e39d6) Co-authored-by: Jack DeVries <58614260+jdevries3133@users.noreply.github.com>

Uh oh!

Conversation

jdevries3133 commented Jul 2, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akulakov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jdevries3133 commented Jul 2, 2021

Uh oh!

akulakov commented Jul 2, 2021 via email

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdevries3133 commented Jul 2, 2021

Uh oh!

akulakov commented Jul 2, 2021 via email

Uh oh!

akulakov commented Jul 3, 2021

Uh oh!

akulakov commented Jul 7, 2021

Uh oh!

jdevries3133 commented Jul 7, 2021

Uh oh!

akulakov commented Jul 7, 2021

Uh oh!

merwok commented Jul 9, 2021

Uh oh!

akulakov commented Jul 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

akulakov commented Jul 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merwok commented Jul 28, 2021

Uh oh!

akulakov commented Jul 28, 2021

Uh oh!

jdevries3133 commented Jul 28, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

jdevries3133 commented Jul 2, 2021 •

edited by bedevere-bot

Loading

jdevries3133 commented Jul 28, 2021 •

edited by bedevere-bot

Loading