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

difflib should separate filename from timestamp with tab #51834

Closed
techtonik mannequin opened this issue Dec 27, 2009 · 21 comments
Closed

difflib should separate filename from timestamp with tab #51834

techtonik mannequin opened this issue Dec 27, 2009 · 21 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@techtonik
Copy link
Mannequin

techtonik mannequin commented Dec 27, 2009

BPO 7585
Nosy @tim-one, @pitrou, @bitdancer, @briancurtin
Files
  • difflib.tab_separated_filename.diff
  • example.py
  • issue7585.difflib-tab-separator.diff: base patch
  • issue7585.difflib-tab-separator-no-trail.diff: no trailing whitespace
  • issue7585.difflib-tab-update-docs.diff: (optional) update docs
  • issue7585.difflib-tab-updoc-iso8601.diff: (opt) alt docs update
  • issue7585.difflib-tab-unittest.diff: with unittest
  • 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/bitdancer'
    closed_at = <Date 2010-04-12.16:59:46.784>
    created_at = <Date 2009-12-27.20:57:36.639>
    labels = ['type-bug', 'library']
    title = 'difflib should separate filename from timestamp with tab'
    updated_at = <Date 2010-05-13.16:24:26.892>
    user = 'https://bugs.python.org/techtonik'

    bugs.python.org fields:

    activity = <Date 2010-05-13.16:24:26.892>
    actor = 'techtonik'
    assignee = 'r.david.murray'
    closed = True
    closed_date = <Date 2010-04-12.16:59:46.784>
    closer = 'r.david.murray'
    components = ['Library (Lib)']
    creation = <Date 2009-12-27.20:57:36.639>
    creator = 'techtonik'
    dependencies = []
    files = ['15682', '15691', '16730', '16731', '16733', '16734', '16797']
    hgrepos = []
    issue_num = 7585
    keywords = ['patch']
    message_count = 21.0
    messages = ['96924', '96961', '96980', '96986', '96989', '97013', '97018', '97164', '97166', '99167', '102046', '102154', '102160', '102318', '102323', '102530', '102551', '102958', '103011', '103041', '105641']
    nosy_count = 5.0
    nosy_names = ['tim.peters', 'pitrou', 'techtonik', 'r.david.murray', 'brian.curtin']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue7585'
    versions = ['Python 2.7', 'Python 3.2']

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Dec 27, 2009

    The patch inserts \t character between filename and timestamp in unified
    and context diff headers.

    According to specification by Guido Van Rossum =)
    http://www.artima.com/weblogs/viewpost.jsp?thread=164293

    And de-facto output from various tools
    http://code.google.com/p/python-patch/source/browse/#svn/trunk/doc

    And the common sense
    --- that it is easier to split this stuff
    +++ than this one into filename + timestamp

    --- diff.py Sun Dec 27 16:08:28 2009
    +++ trunk/diff.py Sun Dec 27 15:46:58 2009

    @techtonik techtonik mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Dec 27, 2009
    @briancurtin
    Copy link
    Member

    I don't think the conditional checks around the timestamps are
    necessary. Couldn't you just put the \t directly in the string that gets
    yielded? That way the chunk headers always follow the same format.

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Dec 29, 2009

    Conditional checks are required to prevent leaving trailing whitespace in
    filename when date component is not present. Such trailing whitespace may
    confuse patch tools. [1]

    [1] http://code.google.com/p/python-patch/issues/detail?id=2

    @briancurtin
    Copy link
    Member

    Shouldn't the patch tool handle that? FWIW, both the "svn diff" and *nix
    diff utilities produce headers with the parts split by a tab character.

    Would the code in example.py work in your tool to handle both tabs and
    spaces?

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Dec 29, 2009

    Filenames may contain spaces too.

    --- handle dis Sun Dec 27 16:08:28 2009
    --- или вот это пон, дек 27 16:08:28 2009

    The last line is space separated filename and date in Russian locale.
    Patch tool should handle that, but as you may see it is not always
    possible. That's why difflib modification with \t separator will greatly
    improve interoperability of difflib patches regardless of timestamp
    format.

    Stripping trailing whitespace when there is no timestamp serves the same
    purpose. We can assume that patch tools are perfect, but I wouldn't
    write my tool if that was true, so its better to be friendly on difflib
    side and generate the output that won't require more work than necessary
    to use it.

    @briancurtin
    Copy link
    Member

    I agree that splitting with a tab character is good, but I think it
    should be split by a tab character in every case.

    If the separator is different based on what data is provided, then it
    complicates parsing and would have to be explained in documentation that
    we provide different header formats.

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Dec 30, 2009

    This patch makes sure filename and date split by tab in every case when
    date is provided.

    @pitrou
    Copy link
    Member

    pitrou commented Jan 3, 2010

    Is there any reason why you changed some of the examples in the docstrings (e.g. 'Sat Jan 26 23:30:50 1991' -> '1991-01-26 23:30:50')?

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Jan 3, 2010

    It is the same reason as for removing recommendation from docstring to generate timestamps in the format returned by time.ctime(). See issue bpo-7582

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Feb 10, 2010

    The reason is to provide a good usage example.

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Mar 31, 2010

    depends on issue bpo-7583

    @bitdancer bitdancer changed the title [patch] difflib should separate filename from timestamp with tab difflib should separate filename from timestamp with tab Apr 1, 2010
    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Apr 2, 2010

    Refreshed patches - feel free to apply in any order you want.

    Convenience link for reviews:
    http://codereview.appspot.com/809043/show

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Apr 2, 2010

    These patches are incrementally add features. Feel free to apply one that suits most. Codereview provides more convenient way to review differences between these.

    attaching optional patch that removes recommendation to use ctime format for timestamps

    @bitdancer
    Copy link
    Member

    Your last patch looks the best to me. I agree both that a tab should not be emitted if there is no date (which is what git, for example, does), and that ISO 8601 timestamps should be promoted as the preferred format.

    As you pointed out, bpo-7583 needs to be resolved before this can be applied.

    In the meantime it would be nice to add an additional test for the no-tab-if-no-date case.

    @bitdancer bitdancer self-assigned this Apr 4, 2010
    @bitdancer
    Copy link
    Member

    We could avoid the 7583 problem by making the doctests use NORMALIZE_WHITESPACE and moving the real *tests* into the unittests for the module. I think that would be a good thing to do anyway.

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Apr 7, 2010

    Added unittest for tab with and without set filedate.
    Removed bpo-7583 dependency with NORMALIZE_WHITESPACE.

    @bitdancer
    Copy link
    Member

    Great, thanks. I'll check this in when the branch is unfrozen.

    @bitdancer
    Copy link
    Member

    Applied to trunk in r8004 and py3k in r8006.

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Apr 13, 2010

    r80004 and r80006 to be exact.

    Great! Thanks! =)

    @bitdancer
    Copy link
    Member

    Yes, thanks for the typo correction.

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented May 13, 2010

    tag:difflib

    @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-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants