Skip to content
New issue

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

Add textwrap.indent() as counterpart to textwrap.dedent() #58065

Closed
ncoghlan opened this issue Jan 25, 2012 · 22 comments
Closed

Add textwrap.indent() as counterpart to textwrap.dedent() #58065

ncoghlan opened this issue Jan 25, 2012 · 22 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@ncoghlan
Copy link
Contributor

BPO 13857
Nosy @birkenfeld, @amauryfa, @ncoghlan, @bitdancer, @cjerdonek
Files
  • indent.patch: patch implementing textwrap.indent, plus tests and documentation
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/ncoghlan'
    closed_at = <Date 2012-06-11.13:31:24.668>
    created_at = <Date 2012-01-25.03:37:15.250>
    labels = ['type-feature', 'library']
    title = 'Add textwrap.indent() as counterpart to textwrap.dedent()'
    updated_at = <Date 2012-06-12.15:00:38.252>
    user = 'https://github.com/ncoghlan'

    bugs.python.org fields:

    activity = <Date 2012-06-12.15:00:38.252>
    actor = 'chris.jerdonek'
    assignee = 'ncoghlan'
    closed = True
    closed_date = <Date 2012-06-11.13:31:24.668>
    closer = 'ncoghlan'
    components = ['Library (Lib)']
    creation = <Date 2012-01-25.03:37:15.250>
    creator = 'ncoghlan'
    dependencies = []
    files = ['24374']
    hgrepos = []
    issue_num = 13857
    keywords = ['patch']
    message_count = 22.0
    messages = ['151932', '151938', '151940', '151945', '151946', '152230', '152231', '152343', '152346', '152363', '152477', '152482', '160505', '160507', '161879', '161885', '161890', '161893', '161917', '162613', '162615', '162681']
    nosy_count = 11.0
    nosy_names = ['georg.brandl', 'amaury.forgeotdarc', 'ncoghlan', 'r.david.murray', 'chris.jerdonek', 'elsdoerfer', 'rutsky', 'python-dev', 'dontknow', 'brandjon', 'ezberch']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue13857'
    versions = ['Python 3.3']

    @ncoghlan
    Copy link
    Contributor Author

    As far I am aware, the simplest way to indent a multi-line string is with the following snippet:

    '\n'.join((4 * ' ') + x for x in s.splitlines())
    

    It would be a lot simpler and clearer if I could just write that as "textwrap.indent(s, 4 * ' ')".

    (i.e. indent would accept a prefix string to be inserted before each line in the supplied string, as in the original comprehension)

    @ncoghlan ncoghlan added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Jan 25, 2012
    @ncoghlan
    Copy link
    Contributor Author

    David Miller pointed out a shorter spelling:

        s.replace('\n', '\n' + (4 * ' '))

    Still not particularly obvious to the reader (or writer), though.

    @amauryfa
    Copy link
    Member

    If such a function is added, I'd like the option to not indent empty lines: trailing spaces are often not a good idea.

    @ncoghlan
    Copy link
    Contributor Author

    I'd actually suggest that as the default behaviour (and is a good argument in favour of a dedicated function in textwrap - both suggested alternatives will blithely add whitespace to otherwise empty lines).

    To handle the empty line requires either switching to an re.sub() based solution or adding a conditional expression:
    '\n'.join(((4 * ' ') + x if x else x) for x in s.splitlines())

    I should probably also explicitly address the "why not textwrap.fill()?" alternative: because fill() does a lot more than simple indenting.

    @brandjon
    Copy link
    Mannequin

    brandjon mannequin commented Jan 25, 2012

    If such a function is added, I'd like the option to not indent empty lines: trailing spaces are often not a good idea.

    From dedent's documentation, it wasn't immediately clear to me that it ignores blank lines when determining common whitespace. (In fact the comment in the example suggests otherwise.) Perhaps a note could be added to the documentation when this change is made?

    @birkenfeld
    Copy link
    Member

    BTW, the short spelling looks like it wouldn't indent the first line.

    @birkenfeld
    Copy link
    Member

    Otherwise +1.

    @dontknow
    Copy link
    Mannequin

    dontknow mannequin commented Jan 30, 2012

    Just wondering if someone is already working on this or am I free to supply a patch?

    @ncoghlan
    Copy link
    Contributor Author

    Please go ahead!

    And Georg is right - the short spelling doesn't handle the first line correctly. It also suffers from the "trailing whitespace" problem that Amaury pointed out in my original version.

    The tests for the new function should check both those cases (i.e. don't indent blank lines, ensure the first line is correctly indented).

    @ezberch
    Copy link
    Mannequin

    ezberch mannequin commented Jan 31, 2012

    I've created a patch using the conditional expression in msg151945. The one problem I found with it is that when the input string is terminated by a newline it removes that newline.

    I've added an optional third argument: a function which determines which lines are indented. If omitted, the default behavior is to indent non-empty lines.

    @birkenfeld
    Copy link
    Member

    IMO removing trailing newlines is not acceptable. You could use splitlines(keepends=True) to keep final newlines (but then the default function that determines lines to indent needs to ignore these newlines).

    @ezberch
    Copy link
    Mannequin

    ezberch mannequin commented Feb 2, 2012

    Sorry, I guess I wasn't clear. The trailing-newlines issue was an issue with the conditional expression ncoghlan suggested. It's fixed in the patch I submitted (and covered by the tests).

    @cjerdonek
    Copy link
    Member

    I'd like to see this enhancement as well. It seems that not even a TextWrapper is capable of a simple indent (because TextWrapper methods operate on "paragraphs" rather than lines).

    @cjerdonek
    Copy link
    Member

    Should the function work for strings with non-Unix line endings?

    http://docs.python.org/dev/py3k/reference/lexical_analysis.html#physical-lines

    For example, should indent("abc\r\n", "") return the same string, and should "\r\n" get indented by default?

    @ncoghlan
    Copy link
    Contributor Author

    Added some review comments. I'm thinking the docs for str.splitlines() could really do with an update to say explicitly that a trailing newline *doesn't* append an empty string to the result.

    @bitdancer
    Copy link
    Member

    Why would you expect it to?

    >>> 'a\nb'.splitlines()
    ['a', 'b']
    >>> 'a\nb\n'.splitlines()
    ['a', 'b']
    >>> 'a\nb\n\n'.splitlines()
    ['a', 'b', '']

    That's exactly what I would intuitively expect, and I don't see how it could possibly do anything else.

    @cjerdonek
    Copy link
    Member

    Perhaps because that's what str.split() does:

    >>> "a\nb".split("\n")
    ['a', 'b']
    >>> "a\nb\n".split("\n")
    ['a', 'b', '']
    >>> "a\nb\n\n".split("\n")
    ['a', 'b', '', '']

    @bitdancer
    Copy link
    Member

    That's why it's a different function :) (Well, that and universal newline support). But I can see that explaining the difference between split and splitlines would be worthwhile.

    @ncoghlan
    Copy link
    Contributor Author

    I created bpo-14957 to cover improving the str.splitlines docs.

    For this patch, I think Chris is right that it should be using str.splitlines(True) and joining on "''" instead of "'\n'" so that Windows line endings get preserved.

    @ncoghlan ncoghlan self-assigned this Jun 11, 2012
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 11, 2012

    New changeset 6f7afe25d681 by Nick Coghlan in branch 'default':
    Close bpo-13857: Added textwrap.indent() function (initial patch by Ezra
    http://hg.python.org/cpython/rev/6f7afe25d681

    @python-dev python-dev mannequin closed this as completed Jun 11, 2012
    @ncoghlan
    Copy link
    Contributor Author

    Ezra (and anyone interested) may want to take a look at the checked in version to see some of the changes I made while preparing the patch for commit.

    • name changes and slight restructure as discussed on the review
    • splitlines() invocation changed as discussed above
    • doc examples changed to doctest style
    • tests reworked to use a parameterised style (taking the easy way out of just failing on the first broken case, since there aren't that many cases and the test is quick to run)
    • default predicate reworked to round trip with textwrap.dedent

    @ncoghlan ncoghlan reopened this Jun 11, 2012
    @cjerdonek
    Copy link
    Member

    Great. Looks good!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants