Closed
Bug 985606
Opened 10 years ago
Closed 10 years ago
mozlog.structured.reader.map_action doesn't document that it's a generator
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla31
People
(Reporter: ted, Assigned: ted)
Details
Attachments
(2 files)
3.11 KB,
patch
|
jgraham
:
review-
|
Details | Diff | Splinter Review |
9.38 KB,
patch
|
ahal
:
review+
|
Details | Diff | Splinter Review |
In fact, the docstring shows an example that *doesn't work* because nothing iterates over the generator. This is a pretty big footgun. I would suggest we make it not a generator, since that doesn't seem to add much value on top of the reader.read function. Either way the docs need to match the implementation.
Assignee | ||
Comment 1•10 years ago
|
||
This fixes the implementation to match the documentation, and adds a simple test.
Attachment #8393718 -
Flags: review?(james)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → ted
Status: NEW → ASSIGNED
Comment 2•10 years ago
|
||
Comment on attachment 8393718 [details] [diff] [review] make mozlog.structured.reader.map_action not be a generator Review of attachment 8393718 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mozbase/mozlog/mozlog/structured/reader.py @@ +28,5 @@ > with actions not in this dictionary will be skipped. > """ > for item in log_iter: > if item["action"] in action_map: > + action_map[item["action"]](item) So I don't think that this should be called action_map if it doesn't actually return anything; instead it should probably be called log_each or something. I wonder if we should have log_each, log_map (like the current map_action) and something like process_log which you pass an object implementing the "log visitor" interface (i.e. one with a handle_foo method for each action foo that it wants to handle). Or maybe for the latter case we should just hoist the base class in formatters into this module.
Comment 3•10 years ago
|
||
So my proposal looks more like [1]. Note this is untested, but if you agree it's a better idea I can work it into a proper patch. [1] https://github.com/jgraham/gecko-dev/commit/0a6fa87021fabb7b08545a68cd69c15a2116a234
Comment 4•10 years ago
|
||
Comment on attachment 8393718 [details] [diff] [review] make mozlog.structured.reader.map_action not be a generator Review of attachment 8393718 [details] [diff] [review]: ----------------------------------------------------------------- Let's do this the other way; I pretty much have a patch.
Attachment #8393718 -
Flags: review?(james) → review-
Comment 5•10 years ago
|
||
Attachment #8399897 -
Flags: review?(ahalberstadt)
Comment 6•10 years ago
|
||
Comment on attachment 8399897 [details] [diff] [review] Improve API for reading structured logs. Review of attachment 8399897 [details] [diff] [review]: ----------------------------------------------------------------- Nice, this looks good. Will definitely come in handy for a project I'm working on. r+ with some optional suggestions. ::: testing/mozbase/mozlog/mozlog/structured/reader.py @@ +23,5 @@ > raise > > > +def imap_log(log_iter, action_map): > + """Create an iterator that will a callback per action for each item in a nit: will call a callback? @@ +44,5 @@ > + with actions not in this dictionary will be skipped. > + """ > + for item in log_iter: > + if item["action"] in action_map: > + action_map[item["action"]](item) Can't this function just be 'list(imap_log(log_iter, action_map))'? Also it might sometimes be useful to return a list of return values. For example an application could do validation on each line like: # callbacks in action_map return True/False if all(reader.each_log(log_iter, action_map)): ...
Attachment #8399897 -
Flags: review?(ahalberstadt) → review+
Comment 7•10 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #6) > @@ +44,5 @@ > > + with actions not in this dictionary will be skipped. > > + """ > > + for item in log_iter: > > + if item["action"] in action_map: > > + action_map[item["action"]](item) > > Can't this function just be 'list(imap_log(log_iter, action_map))'? Also it > might sometimes be useful to return a list of return values. For example an > application could do validation on each line like: Well that requires that you return a value for each item, and construct a list, which you might not want to do for reasons of e.g. memory consumption. The closest analogue to this function is actually something like: for item in imap_log(iter, action_map): pass But the error-proneness of that pattern was the original motivation for this bug. > # callbacks in action_map return True/False > if all(reader.each_log(log_iter, action_map)): Seems like that already works with imap_log, no? All in all I'm inclined to leave this as-is for now.
https://hg.mozilla.org/mozilla-central/rev/69b7bd12aa94
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•