Oh, okay. Sounds good. <div class="gmail_quote">On Apr 15, 2013 12:28 AM, "Steven Hiscocks" <<a href="mailto:steven-systemd@hiscocks.me.uk">steven-systemd@hiscocks.me.uk</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On 15/04/13 04:05, David Strauss wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Maybe I'm missing something in the Gist patch, but won't Reader.get('__REALTIME_TIMESTAMP') fail with "field name is not valid"? </blockquote> You're right, but `get` is changed to `_get`, so is "private". Use `get_next` and `get_previous` does on the traversal and getting of both special and standard fields to simplify use of the Reader. <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On Sun, Apr 14, 2013 at 10:49 AM, Zbigniew Jędrzejewski-Szmek <<a href="mailto:zbyszek@in.waw.pl" target="_blank">zbyszek@in.waw.pl</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On Sun, Apr 14, 2013 at 03:28:31PM +0100, Steven Hiscocks wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On 14/04/13 03:36, David Strauss wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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. </blockquote></blockquote> 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. <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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. </blockquote> 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: <a href="https://gist.github.com/kwirk/5382783" target="_blank">https://gist.github.com/kwirk/5382783</a> </blockquote> 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? ;) <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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...) </blockquote> 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 </blockquote> </blockquote> -- Steven Hiscocks </blockquote></div>