feat: add support for optional pulsetime param to flood in query2#135
feat: add support for optional pulsetime param to flood in query2#135
Conversation
Greptile SummaryThis PR adds an optional
Confidence Score: 4/5Safe to merge; the wrapper change is minimal and backwards-compatible, with the only gap being that the query2 decorator chain is untested with the new parameter. The wrapper change is a one-liner with a default that preserves existing behaviour, and the underlying tests/test_flood.py — consider adding a test that calls Important Files Changed
Sequence DiagramsequenceDiagram
participant Query as query2 engine
participant q2_flood
participant flood as aw_transform.flood()
Query->>q2_flood: "flood(events, pulsetime=31)"
Note over q2_flood: q2_function strips datastore/namespace args
Note over q2_flood: q2_typecheck validates required args only
q2_flood->>flood: "flood(events, pulsetime=31)"
flood-->>q2_flood: "List[Event] (gaps <= 31s filled)"
q2_flood-->>Query: List[Event]
Reviews (1): Last reviewed commit: "feat: add support for optional pulsetime..." | Re-trigger Greptile |
| def test_flood_with_custom_pulsetime(): | ||
| """Test that flood respects custom pulsetime parameter""" | ||
| # Create events with 30s gap (simulating 30s poll_time) | ||
| events = [ | ||
| Event(timestamp=now, duration=5, data={"a": 0}), | ||
| Event(timestamp=now + 35 * td1s, duration=5, data={"b": 1}), | ||
| ] | ||
|
|
||
| # With default pulsetime=5, gap should NOT be flooded | ||
| flooded_default = flood(events) | ||
| assert len(flooded_default) == 2 | ||
| total_duration_default = sum((e.duration for e in flooded_default), timedelta(0)) | ||
| assert total_duration_default == timedelta(seconds=10) | ||
|
|
||
| # With pulsetime=31 (matching 30s poll_time + 1), gap should be flooded | ||
| flooded_custom = flood(events, pulsetime=31) | ||
| assert len(flooded_custom) == 2 | ||
| total_duration_custom = sum((e.duration for e in flooded_custom), timedelta(0)) | ||
| assert total_duration_custom == timedelta(seconds=40) # Full duration with gap filled |
There was a problem hiding this comment.
Test covers
flood() directly, not q2_flood through the query2 layer
The new test validates the underlying flood() function with pulsetime, but it doesn't verify that the parameter is correctly forwarded through the q2_flood wrapper and its decorator chain (q2_function + q2_typecheck). Since q2_function manipulates *args by stripping datastore/namespace arguments positionally, a positional-index shift bug in the decorator could cause pulsetime to be silently ignored without this test catching it. There are no existing q2_flood tests in test_query2.py to compensate.
I had this in a dirty working tree, can't remember why I added it. But kinda makes sense?