Skip to content

Conversation

@ThomasWaldmann
Copy link
Contributor

note: this needs bug #282 to be fixed first, because it will create timestamps with microseconds==0.

Without the #282 bug fix, one can work around by always giving a non-zero fraction of a second in the timestamps, e.g. 2015-04-19T16:14:15.1Z.

fixes #263.

… file/dir

note: this needs bug #282 to be fixed first, because it will create timestamps with microseconds==0.
@camh-
Copy link

camh- commented Apr 18, 2015

Hmm, that's annoying that github puts each of my line comments as a separate pull request comment. It makes it rather noisy and loses the context of the comments. (If you can't tell, this is my first github code review as a reviewer).

@camh-
Copy link

camh- commented Apr 19, 2015

I really feel it is wrong to force users to use UTC. datetimes should always be stored in UTC, but user interaction should default to their local time (with an option for UTC if the user desires).

Allowing only UTC timestamp strings shows up as an incongruity when listing the archives in a repository. The output of 'attic list ' explicitly converts archive timestamps to local time (see helpers.py:format_archive() and helpers.py:to_localtime()).

I do not feel it is necessary to support arbitrary timezones (just localtime and UTC), and this can be achieved without external dependencies (such as pytz which is usually called in as soon as you need to deal with multiple timezones). Implementing an equivalent helper to to_localtime (from_localtime) could be done to support this.

@ThomasWaldmann
Copy link
Contributor Author

@camh- thanks for the feedback, I'll see how I can improve it.

…O8601 format

support YYYY-MM-DDThh:mm:ss.ffffffZ format - or less precise versions of it.

always require Z suffix as we only accept UTC timestamps for now.
@ThomasWaldmann
Copy link
Contributor Author

@camh- I agree with all what you said and improved the code.

BUT: tz support sucks. :| Thus, I did not add localtime or TZ support. I didn't want to require additional dependencies or complicate the code too much.

@camh-
Copy link

camh- commented Apr 19, 2015

Generic TZ handling is painful (and requires something like pytz as an additional dependency), but local time support is not too hard. How about something like (untested in totality):

def from_localtime(dt):
  seconds = time.mktime(dt.timetuple())
  return datetime.utcfromtimestamp(seconds).replace(microsecond=dt.microsecond)

And the body of the time string conversion:

import itertools

dates = ['%Y-%m-%d', '%Y-%j']
times = ['T%H:%M:%S.%f', 'T%H:%M:%S', 'T%H:%M', 'T%H', '']
zones = ['', 'Z']
for format in itertools.product(dates, times, zones):
  try:
    dt = datetime.strptime(s, ''.join(format))
    return dt if format[2] == 'Z' else from_localtime(dt)
  except ValueError:
    pass

@jborg
Copy link
Owner

jborg commented Apr 19, 2015

Generic TZ handling is painful (and requires something like pytz as an additional dependency), but local time support is not too hard. How about something like (untested in totality):

Agreed, to_localtime uses a similar trick.

@ThomasWaldmann
Copy link
Contributor Author

@camh- nice trick with the product. I'll work on integrating that and localtime support. BTW, I kept the "T" separate in case someone wants rather a space than a T, but that can be done in the same way.

also:
- use itertools.product instead of nested loops
- update help
- remove support for fractions of a second

note: from_localtime and to_localtime both drop the microseconds,
this should get a global review and be dealt with in a separate commit.
@ThomasWaldmann
Copy link
Contributor Author

OK, updated.

While doing that, found a slight issue with to_localtime - it does not support fractions of a second.
Chose to do the same with from_localtime and in general not to support fractions of a second with --timestamp (likely noone needs that anyway).

@ThomasWaldmann
Copy link
Contributor Author

closing this pull request, seems unwanted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Specify the date and time of the backup

3 participants