Maybe I'm missing something in the Gist patch, but won't
Reader.get('__REALTIME_TIMESTAMP') fail with "field name is not
valid"?On Sun, Apr 14, 2013 at 10:49 AM, Zbigniew Jędrzejewski-Szmek <[email protected]> wrote: > On Sun, Apr 14, 2013 at 03:28:31PM +0100, Steven Hiscocks wrote: >> On 14/04/13 03:36, David Strauss wrote: >> >I keep writing lengthy emails about how we can use this as an >> >opportunity to reduce redundancy and improve consistency, but I should >> >probably ping you on #systemd IRC to hash it out. I can't think of >> >anything elegant that doesn't involve altering the existing journal.py >> >or _reader.c code. > Hi Steven, hi David, > > I read your discussion on IRC... I agree that backwards compatibility > is not (yet) something that we need to keep very. I think that the > number of people using those Python interfaces is quite small so far, > and having a nice interface is more important than some small breakage. > >> >As a starter, I want to enumerate the places where >> >__REALTIME_TIMESTAMP has special handling: >> > (1) Existing code: A C-based member function in _reader.c. >> > (2) Existing code: A native Python conversion function in journal.py. >> > (3) Existing code: An entry in DEFAULT_CONVERTERS. >> > (4) Added in your patch: A key setting in get_next(). >> > (5) Added in your patch: A Python-based member function in journal.py >> >that overrides (1). >> > (6) Added in your patch: A condition in get() that invokes (1) when >> >the specific field is requested. >> > >> >While I want to fix this bug, I also don't want six scattered special >> >cases for __REALTIME_TIMESTAMP and similar fields. >> > >> >> Thanks for the chat earlier on IRC David, especially given your >> localtime :-). >> >> I thought about the points discussed: keeping things similar to >> journalctl -o json output (i.e. include special fields); get, >> get_next, and get_realtime, etc methods create duplication and >> multiple ways to get the same/some values (could cause confusion); >> issue with `next` clashing with python2 __iter__ next; trying to >> stabilise the interface (note: not declared stable yet); performance >> issues with unnecessary dictionary field conversions. >> >> I've therefore created another patch for consideration: >> https://gist.github.com/kwirk/5382783 > This looks pretty nice, and I think it can be committed. Could > you create a patch with a commit message, including some of the > rationale, so that people understand what's happenning? ;) > >> I've removed the get_next and get_previous methods from _Reader, as >> these didn't align with the C API. This is replaced with:`_get_all` >> private method (effectively a wrapper around C API macro >> sd_journal_{enumerate,restart}_data/SD_JOURNAL_FOREACH_DATA); >> renamed `next` method to `_next`, such to avoid name clash (and I >> believe should be private); `_previous` method for consistency. >> >> The `get_next` and `get_previous` methods are now in Reader only. >> These call `_next` method, followed by `_get_all` and then fetch the >> special fields, before passing whole dictionary to converters. This >> makes all the traversal, and getting standard and special fields >> from the journal transparent to the user. >> >> With the changes of removing `get_next` in _Reader, the iter methods >> don't function as you'd expect. To that end, I've moved them to >> Reader with get_next as iter next call as before. >> >> I understand the points about performance, but don't think the >> performance hit is that bad, or critical in most use cases. These >> changes should allow a custom class to inherit _Reader, and the >> create a more optimised version if that is required. (One option if >> this is a major issue, is a custom dictionary class could be >> returned by `get_next`, which uses the __get__ method to lazy >> convert the fields on access. This is starting to get complicated >> thought...) > I think we can always do that later on. Maybe it makes sense > to document that get_next() returns something dictionary-like > (collections.abc.Mapping) that right now is just a dict, but > could become something different later on. > > Zbyszek -- David Strauss | [email protected] | +1 512 577 5827 [mobile] _______________________________________________ systemd-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
