Skip to content

Conversation

@mbirtwell
Copy link
Contributor

The exposes some of the behaviour of format_datetime which might be useful
on it's own as it's own function.

Although this doesn't require the changes I proposed in #265 it is complementary to them.

It allows you to format the date and time using different or explicit formats and then combine them in a locale aware way.

The exposes some of the behaviour of format_datetime which might be useful
on it's own as it's own function.
@codecov-io
Copy link

Current coverage is 83.45%

Merging #267 into master will increase coverage by +0.01% as of ab93d1a

@@            master    #267   diff @@
======================================
  Files           21      21       
  Stmts         3794    3796     +2
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           3166    3168     +2
  Partial          0       0       
  Missed         628     628       

Review entire Coverage Diff as of ab93d1a

Powered by Codecov. Updated on successful CI builds.

@etanol
Copy link
Contributor

etanol commented Sep 29, 2015

I'm sorry, but no. This implementation cannot handle timezone support at all. Trying to concatenate date and time on the wild is just asking for trouble. That is why we have a datetime type.

@etanol etanol closed this Sep 29, 2015
@mbirtwell
Copy link
Contributor Author

Well with out a function like this you are making life hard for someone who wants a long date but to only give the time to the nearest minute. Very reasonable for example if you are showing the start time of a meeting. It could be implemented with my function like:

def format_mydatetime(ts, locale):
    return format_date_with_time(
        format_date(ts, 'full', locale=locale),
        format_time(ts, 'short', locale=locale),
        'long',
        locale,
    )

format_date and format_time are both tz aware and assuming that ts is a datetime with the appropriate time zone info this will just work. I admit it requires the user to do things properly so maybe it should come with a health warning, but it's not too hard to do so and if you don't provide this function people will just have to implement potentially buggy copies of it instead.

@etanol
Copy link
Contributor

etanol commented Sep 29, 2015

It's just bad API design. Formatting functions should accept native datatypes and convert them to localized strings. I don't see the justification in adding a function that can accept arbitrary strings and returns another (partially localized) string.

I'm very sorry about this because I've been in your position several times. But this one is not going to happen.

@mbirtwell
Copy link
Contributor Author

How would you feel about a pull request that augmented format_datetime instead so that it provided options for using different formats for the date and the time?

@etanol
Copy link
Contributor

etanol commented Sep 30, 2015

That is certainly more reasonable. For example, optionally accepting a tuple or list from the format parameter. It would keep compatibility, being a very subtle API change, and you would get your desired functionality with almost no code duplication.

@mbirtwell
Copy link
Contributor Author

So I was thinking prototype would be something like

format_datetime(datetime=None, format='medium', tzinfo=None, locale=LC_TIME, date_fmt=None, time_fmt=None)

where date_fmt and time_fmt would be ignored if format is not one of ('full', 'long', 'medium', 'short')
date_fmt and time_fmt would default to format if not set
date_fmt and time_fmt would be passed to format_date and format_time appropriately.

date_fmt and time_fmt could the each independantly be one of the standard format names or a custom format. The one thing that it seems to me this misses is support for skeletons (which you might have noticed I'm keen on). But it's easy enough to look up the skeleton from the locale object and pass this in as a custom format.

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.

4 participants