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

Use ISO timestamp in diff.py #51831

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

Use ISO timestamp in diff.py #51831

techtonik mannequin opened this issue Dec 27, 2009 · 29 comments
Assignees
Labels
type-feature A feature request or enhancement

Comments

@techtonik
Copy link
Mannequin

techtonik mannequin commented Dec 27, 2009

BPO 7582
Nosy @warsaw, @amauryfa, @abalkin, @djc, @merwok
Dependencies
  • bpo-9527: Add aware local time support to datetime module
  • bpo-1647654: No obvious and correct way to get the time zone offset
  • Files
  • diff.py_iso_timestamps.diff
  • diff.py_iso_timestamps_true.diff: flawless
  • diff.py_iso_timestamps_true_with_dst.diff: also detects dst
  • diff.py_iso_timestamps_true_with_true_dst.diff: active dst
  • 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/abalkin'
    closed_at = <Date 2012-06-22.17:37:53.631>
    created_at = <Date 2009-12-27.15:57:56.577>
    labels = ['type-feature']
    title = 'Use ISO timestamp in diff.py'
    updated_at = <Date 2012-06-22.18:00:30.271>
    user = 'https://bugs.python.org/techtonik'

    bugs.python.org fields:

    activity = <Date 2012-06-22.18:00:30.271>
    actor = 'techtonik'
    assignee = 'belopolsky'
    closed = True
    closed_date = <Date 2012-06-22.17:37:53.631>
    closer = 'belopolsky'
    components = ['Demos and Tools']
    creation = <Date 2009-12-27.15:57:56.577>
    creator = 'techtonik'
    dependencies = ['9527', '1647654']
    files = ['15678', '15689', '15798', '16184']
    hgrepos = []
    issue_num = 7582
    keywords = ['patch', 'needs review']
    message_count = 29.0
    messages = ['96912', '96922', '96929', '96962', '96978', '96979', '96981', '97272', '97460', '97471', '97503', '97539', '97540', '97546', '99092', '99096', '99108', '99163', '99166', '99171', '99200', '99393', '104419', '104430', '104439', '107152', '107154', '163434', '163448']
    nosy_count = 8.0
    nosy_names = ['barry', 'amaury.forgeotdarc', 'belopolsky', 'techtonik', 'djc', 'eric.araujo', 'srumbalski', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue7582'
    versions = ['Python 3.2']

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Dec 27, 2009

    make diff.py produce unified diffs with ISO 8601 timestamps

    Currently generated timestamps are difficult to separate from filename
    when parsing if you don't know that the diff was generated by diff.by
    This patch make diff.py output more standard compliant by including ISO
    8601 timestamps that also conform to RFC 3339 profile for timestamp format
    on the internet.

    @techtonik techtonik mannequin added the type-bug An unexpected behavior, bug, or error label Dec 27, 2009
    @amauryfa
    Copy link
    Member

    Using ISO format certainly makes sense, but it seems to me that after
    dt=datetime.fromtimestamp(mtime), dt.tzinfo is always None, so the test is
    not necessary.

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Dec 27, 2009

    That's true. Is there any way to get current TZ offset in Python? I can't
    find anything better than datetime.datetime.now() -
    datetime.datetime.utcnow() and then rounding to a nearest minute.

    @briancurtin
    Copy link
    Member

    Look at time.timezone -
    http://docs.python.org/library/time.html#time.timezone

    # I'm in US Central time, so it's -6 from UTC
    >>> import time
    >>> tz = time.timezone
    >>> tz
    21600
    >>> tz / 60 / 60
    6

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Dec 28, 2009

    Thanks! Now the most elegant patch I could think out.

    @briancurtin
    Copy link
    Member

    Looks cleaner and works for me.

    One very minor comment: the spaces inside the parenthesis on the
    time.localtime call are inconsistent and don't comply with PEP-8.

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Dec 29, 2009

    Pepeighfied and regenerated. Should be flawless now.

    @srumbalski
    Copy link
    Mannequin

    srumbalski mannequin commented Jan 5, 2010

    I think this is incorrect during daylight savings time:
    tzoffset = -time.timezone // 60

    This should do it:
    isdst = time.localtime().tm_isdst
    tzoffset = -(time.altzone if isdst else time.timezone)//60

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Jan 9, 2010

    New version detects DST using time.daylight flag.

    utcoffset = -(time.altzone if time.daylight else time.timezone) // 60

    @briancurtin
    Copy link
    Member

    Using time.daylight is incorrect. time.daylight specifies the number of hours that the daylight offset is, not a flag to specify whether or not daylight savings time is in effect.

    Steven's suggestion of using time.localtime().tm_isdst seems to be the better route.

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Jan 10, 2010

    Brian, documentation says quite the opposite.

    time.daylight
    Nonzero if a DST timezone is defined.
    http://docs.python.org/library/time.html?highlight=daylight#time.daylight

    @briancurtin
    Copy link
    Member

    The documentation could use some work. It means that if the timezone does use a daylight savings time period, time.daylight specifies the amount of the offset. In my timezone this value is 1. However, time.localtime().is_dst is currently 0, because we are not on savings time.

    For that reason, using time.daylight in your patch gives an incorrect result for me.

    @briancurtin
    Copy link
    Member

    http://msdn.microsoft.com/en-us/library/2t504ch6%28VS.80%29.aspx has some info for how this is implemented on Windows, and I get the same results on my Mac.

    On Linux an AttributeError is raised time.struct_time not having an attribute is_dst, and time.daylight matches the other two platforms.

    @briancurtin
    Copy link
    Member

    Whoops, nevermind the Linux comment on that last one. My typo there caused the exception. The result there is the same as the other platforms.

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Feb 9, 2010

    So, is that right that even if time.daylight specifies the offset, this doesn't mean that this offset is active?

    MS documentation you referenced is unclear: "The _get_daylight function retrieves the number of hours in daylight saving time as an integer. If daylight saving time is in effect, the default offset is one hour."

    From this description it is easy to assume that if DST is not in effect then default offset would be 0.

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Feb 9, 2010

    You are right.
    ----[pydst.py]------

    import time

    print time.asctime()
    print time.localtime().tm_isdst
    print time.daylight
    --------------------

    Tue Feb 09 10:31:47 2010
    0
    1

    Sun May 09 10:33:20 2010
    1
    1

    There is already an issue bpo-7229 to correct the docs. I'll adjust my patch accordingly. Thanks for collaboration.

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Feb 9, 2010

    Finally! (I really hope so)

    @warsaw
    Copy link
    Member

    warsaw commented Feb 10, 2010

    This seems like a new feature to me so I'm removing 2.6 from the list. My browser won't let me remove Python 3.1.

    @briancurtin briancurtin added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Feb 10, 2010
    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Feb 10, 2010

    I use this tool in instructions how to generate patches on windows, so I am interested to see this fix in the version, that users will likely to use for next couple of years, but I'd be happy to see this committed in any branch.

    If it is going to be committed in alpha only them I'd like to propose to change default diff format to unicode as well.

    Please, also review http://bugs.python.org/issue7585 that contains more important API change that also prevents diff.py from generating correct patches.

    @amauryfa
    Copy link
    Member

    I'd like to propose to change default diff format to unicode as well
    What kind of "unicode" format is it?

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Feb 11, 2010

    I am sorry - too much windows to reply. This must be *unified*, of course.

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Feb 16, 2010

    Ok. Let's commit it at least to 2.7 - I'll create a separate issue for discussion of unified diff format later.

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Apr 28, 2010

    So, what about 2.7 ?

    @briancurtin
    Copy link
    Member

    2.7 is now frozen as far as new features go. It's still good for 3.2.

    I think this is ready to go, so I'll probably commit it later in the day.

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Apr 28, 2010

    I still do not understand your policy - it is a tool, it is not a part of standard library.

    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Jun 5, 2010

    Adding Alexander as ISTM he may be interested to read the time discussion.

    @abalkin
    Copy link
    Member

    abalkin commented Jun 5, 2010

    The latest patch will produce wrong results if the file was last modified before timezone rules changed in your location. See bpo-1647654.

    @abalkin abalkin self-assigned this Jun 5, 2010
    @merwok merwok changed the title [patch] diff.py to use iso timestamp Use ISO timestamp in diff.py Jul 4, 2010
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 22, 2012

    New changeset ec95b94ea831 by Alexander Belopolsky in branch 'default':
    Issue bpo-7582: Use ISO timestamp in diff.py
    http://hg.python.org/cpython/rev/ec95b94ea831

    @abalkin abalkin closed this as completed Jun 22, 2012
    @techtonik
    Copy link
    Mannequin Author

    techtonik mannequin commented Jun 22, 2012

    Thanks. I am glad OS TZ support finally issue took off. =)

    @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
    type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants