On 25.2.2014 11:15, Tomas Babej wrote:
On 01/14/2014 10:19 AM, Petr Viktorin wrote:
On 01/14/2014 09:27 AM, Jan Cholasta wrote:
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.
On yesterday's meeting, Simo said that on the wire and for CLI output,
we should use generalized time.
I disagree with using it for CLI ouptut, as I find generalized time
unreadable, but for the API it's the obvious choice.
I believe we should require the "Z" postfix in all formats, so that 1)
users are forced to be aware that it's UTC, and 2) we can
theoretically support local time in the future.
I think the supported input formats should be:
%Y%m%d%H%M%SZ (generalized time)
%Y-%m-%dT%H:%M:%SZ (ISO 8601, with seconds)
%Y-%m-%dT%H:%MZ (ISO 8601, without seconds)
%Y-%m-%dZ (ISO 8601, date only)
I also think we should accept a space instead of the "T", as Petr²
said above.
Thanks for review, everybody.
* All accepted formats now require explicit Z
* Accept values with ' ' instead of T
* LDAP generalized time used on the wire with JSON
* Minor implementation fixes
Updated patch should address all the issues.
The first if statement is not necessary here, datetime values are
correctly handled in the super class:
+ def _convert_scalar(self, value, index=None):
+ if isinstance(value, datetime.datetime):
+ return value
+ elif isinstance(value, basestring):
Please use ', ' instead of just ',' as the separator to make the error
message more readable here:
+ error = (_("does not match any of accepted formats: ") +
+ (','.join(self.accepted_formats)))
This if statement is not present on old clients, so the assert below
will fail on them when they receive DateTime:
+ if isinstance(value, DateTime):
+ return datetime.datetime.strptime(str(value), "%Y%m%dT%H:%M:%S")
assert type(value) in (unicode, int, float, bool, NoneType)
But, they will never receive DateTime from us, because LDAP generalized
time is decoded to unicode in ipaldap.
What I think we should do is decode LDAP generalized time to datetime
objects in ipaldap, add a new capability (e.g. "datetime_values") and
use DateTime only for clients with that capability, otherwise use the
generalized time string. Also, something similar should be done for
JSON, so that clients can infer that a value is datetime and not just a
generic string (e.g. wrap it in a dict with '__datetime__', similar to
how binary values are wrapped with '__base64__'), again only for clients
with the capability.
--
Jan Cholasta
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
