On 18/02/13 02:17, David Strauss wrote:
Great progress!
Thanks, and thank you for your feedback.
A few more remarks:
* Class methods ought to be verbs. "this_machine" and "this_boot" are
the key ones that come to mind. Maybe "add_machine_match" and
"add_boot_match"?
Sounds good. I suppose `log_level` should become `add_loglevel_matches`
for consistency. (or actually `add_priority_matches`?)
* In methods like "this_machine" that have special handling for the
None value as "myself," the function should use something more
meaningful. You could even define a constant like CURRENT_MACHINE as
None. It's just unclear in calling code that
myjournal.this_machine(None) matches on the current machine versus,
say, no machine field, an empty machine field, or *any* value in the
machine field.
I was missing the default value of None in `this_machine`. It's supposed
to be used: "myjournal.this_machine()" (or
"myjournal.add_machine_match()" with above changes). Alternatively, the
default None could be removed , and constants set
`CURRENT_MACHINE=id128.get_machine()`, etc., which then should be used
when calling the method?
* "get_next" should note in its documentation that, combined with the
native C, one can use a Pythonic iterator directly. I would even make
this function private to encourage use of the iterator syntax.
I'll add this to the documentation, but I think `get_next` should stay,
as it compliments `get_previous` and use of the `skip` value.
* The "except" handler for _convert_unicode() should be more
specific. I think it's generally dangerous to have catch-all
exceptions, especially ones that fail silently. I couldn't find in the
Python docs for 2 or 3 what the exception would be.
Sure. I'll put in `UnicodeDecodeError`. I'm undecided on whether the
call of `self.converters[key](value)` exception handling should be
limited to `KeyError`? This would mean that the whole log entry would
fail to return if one of the converters failed.
--
David Strauss
| [email protected]
| +1 512 577 5827 [mobile]
On Fri, Feb 15, 2013 at 9:28 AM, Steven Hiscocks
<[email protected]> wrote:
On 11/02/13 05:49, Zbigniew Jędrzejewski-Szmek wrote:
On Fri, Feb 08, 2013 at 10:20:57PM +0000, Steven Hiscocks wrote:
I thought the easiest way was to just inherit the Journal (now
_Journal), replacing __new__ with all the python RunString stuff.
https://github.com/kwirk/systemd/commit/9003fdbc69577840ab6a1501a1c49f7377b6124d
Hi Steven,
comments on your patch:
The default conversion must be bytes, otherwise every new binary
message field that this module doesn't know about will cause failure.
It is better to default to bytes.
Currently I've put default to unicode, which falls back to bytes if their is
an exception is raised.
...
Zbyszek
I've been away on a work trip this week so haven't been able to make much
progress. I've pushed some changes to a branch on github:
https://github.com/kwirk/systemd/tree/python-systemd-reader
--
Steven Hiscocks
--
David Strauss
| [email protected]
| +1 512 577 5827 [mobile]
--
Steven Hiscocks
_______________________________________________
systemd-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
