[Freeipa-devel] [PATCH 0137] ipalib: Add DateTime parameter

[Jan Cholasta]

On 13.1.2014 14:57, Petr Vobornik wrote:
> On 13.1.2014 13:41, Jan Cholasta wrote:
>> Hi,
>>
>> On 10.1.2014 21:21, Nathaniel McCallum wrote:
>>> On Thu, 2014-01-09 at 16:30 +0100, Tomas Babej wrote:
>>>> Hi,
>>>>
>>>> Adds a parameter that represents a DateTime format using
>>>> datetime.datetime
>>>> object from python's native datetime library.
>>>>
>>>> In the CLI, accepts one of the following formats:
>>>> Accepts subset of values defined by ISO 8601:
>>>> %Y-%m-%dT%H:%M:%S
>>>> %Y-%m-%dT%H:%M
>>>> '%Y%m%dT%H:%M:%S'
>>>> '%Y%m%dT%H:%M'
>>>>
>>>> Also accepts LDAP Generalized time in the following format:
>>>> '%Y%m%d%H%M%SZ'
>>>>
>>>> As a simplification, it does not deal with timezone info and ISO 8601
>>>> values with timezone info (+-hhmm) are rejected. Values are expected
>>>> to be in the UTC timezone.
>>>>
>>>> Values are saved to LDAP as LDAP Generalized time values in the format
>>>> '%Y%m%d%H%SZ' (no time fractions and UTC timezone is assumed). To avoid
>>>> confusion, in addition to subset of ISO 8601 values, the LDAP
>>>> generalized
>>>> time in the format '%Y%m%d%H%M%SZ' is also accepted as an input (as
>>>> this
>>>> is the
>>>> format user will see on the output).
>>>>
>>>> Part of: https://fedorahosted.org/freeipa/ticket/3306
>>>
>>> The date/time syntax formats are not compliant with ISO 8601. You stated
>>> they are expected to be in UTC timezone, but no 'Z' is expected in most
>>> of them. This is not only non-standard, but would prevent you from every
>>> supporting local time in the future.
>>
>> +1, please require the trailing "Z".
>>
>>
>> I think generalized time format should be the preferred one, as it is
>> stored in LDAP and thus returned by commands. Also, do we really need
>> all of these other formats?
>
> +1 That API should accept and always return generalized time.
>
> But UIs(CLI, Web) should display something more human readable. Like:
> %Y-%m-%dT%H:%M:%SZ or IMHO better (but not ISO 8601): %Y-%m-%d %H:%M:%SZ
> ie. 2014-03-08 14:16:55Z compared to 2014-03-08T14:16:55Z or
> 20140308141655Z
>
> Both should accept input value in the same format. Usage of different
> output and input formats is confusing. Thus I agree with supporting more
> input formats. Decision whether it should be done on API level or client
> level is another matter.

If you want human readable output, you have to do the conversion from 
generalized time on clients. If you want to support local time and 
timezones, you have to do some processing on the client as well. I would 
be perfectly happy if we supported only generalized time over the wire 
and did conversion from/to human readable on clients.

>
> I has been implementing the Web UI part and I think we should also
> support a formats without time component. My initial impl. supports
> combinations of:
>
>      var dates = [
>          ['YYYY-MM-DD', /^(\d{4})-(\d{2})-(\d{2})$/],
>          ['YYYYMMDD',/^(\d{4})(\d{2})(\d{2})$/]
>      ];
>
>      var times = [
>          ['HH:mm:ss', /^(\d\d):(\d\d):(\d\d)Z$/],
>          ['HH:mm', /^(\d\d):(\d\d)Z$/],
>          ['HH', /^(\d\d)Z$/]
>      ];
> + generalized time
> ['YYYYMMDDHHmmssZ',/^(\d{4})(\d{2})(\d{2})(\d{2})(\d{2})(\d{2})Z$/]
>
> with /( |T)/ as divider and time optional.
>
> Both UIs should also tell the user what is the expected format (at least
> one of them). Getting incorrect format error without knowing it is
> annoying.

The current code raises a ValidationError including the list of formats.

>
>>
>>
>> Few nitpicks about DateTime implementation:
>>
>>    * you should call Param._convert_scalar from DateTime._convert_scalar
>>    * raise ConversionError instead of ValidationError and use
>> get_param_name() for name
>>    * instead of "isinstance(value, str) or isinstance(value, unicode)"
>> use "isinstance(value, basestring)"
>>    * don't use pythonisms in public error messages (""does not match any
>> of accepted formats: %s" % self.accepted_formats")
>>    * public error messages should be translatable
>>
>> It should look something like this (untested, from top of my head):
>>
>>      type = datetime.datetime
>>      type_error = _('must be a date/time value')
>>
>>      def _convert_scalar(self, value, index=None):
>>          if isinstance(value, basestring):
>>              for date_format in self.accepted_formats:
>>                  try:
>>                      time = datetime.datetime.strptime(value,
>> date_format)
>>                      return time
>>                  except ValueError:
>>                      pass
>>
>>              # If we get here, the strptime call did not succeed for any
>>              # the accepted formats, therefore raise error
>>
>>              error = _("does not match any of accepted formats: %s") %
>> (', '.join(self.accepted_formats))
>>
>>              raise ConversionError(name=self.get_param_name(),
>>                                    index=index,
>>                                    error=error)
>>
>>          return super(DateTime, self)._convert_scalar(value, index)
>>
>>
>> +    if isinstance(value, DateTime):
>> +        return str(value)
>>
>> Return datetime object here please, or at least unicode in generalized
>> time format (or whatever the preferred format is, see above).
>>
>>
>> +    elif isinstance(val, datetime.datetime):
>> +        return val.strftime("%Y%m%dT%H:%M:%S")
>>
>> Again, please use the preferred format here please.
>>
>>
>> Honza
>>
>
>


-- 
Jan Cholasta