acsresearch / interlab

MIT License
17 stars 2 forks source link

Context.result not serialized properly if Falsy #17

Closed jas-ho closed 1 year ago

jas-ho commented 1 year ago
@pytest.mark.parametrize("result", [pytest.param(None, id="None"), pytest.param("", id='""'), 0, 1, "abc", {"x": 10}])
def test_set_result(result):
    with Context("root") as c:
        c.set_result(result)
    assert c.result == result
    assert c.to_dict().get("result") == result

fails on empty string and 0, since in those cases c.result is not set in the dict resulting from call to to_dict

jas-ho commented 1 year ago

simple fix: tighten from if value to if value is not None in context.py:122 image

jas-ho commented 1 year ago

...but maybe it's better to just drop line 122 altogether and even explicitly set None values on the output dict?

spirali commented 1 year ago

You are right "is not None" is missing. Probably we can add a flag "has_result" into Context class to serialize .result only if context really has a result to allow "None" as a proper result. But missing result and having state FINISHED or EVENT means result is None. Nevertheless, I do not like idea to create key "result" (in serialized form) for a context that does not actually have a result.

gavento commented 1 year ago

Hey @jas-ho, thanks for the issue and the fix!

A technical note, please prefer rebases to merges:

  1. always when updating your feature branch if main has moved in the meantime (no information is lost here with a rebase except maybe for timing, and errors get caught early), and
  2. oprionally when accepting your branch or PR into main. (this is caller rebase+merge on github. Doing mere or rebase here is more debated, and I prefer rebases, but works for me either way.)

Otherwise history gets this tangled just for a simple fix/improvement. image [...] image