Fix: InsertValue will error if input is None

[mikenerone]

Title says most of it. Related, Metronome handled the no input case correctly, but there wasn't a test for it, so I added one for that, too. I also took the liberty of making the Metronome tests fast with autojump_clock, if that's ok (they were the only ones that slowed the test run).

[andersea]

Ok, so, interesting...

My original intended use case for InsertValue assumes that it will receive further values, in other words it was supposed to not be used as the first section and thus would always get an input. I was never really intending it to be used as first section.

Naively I actually expected that this might cause bad things to happen, like the pipeline might hang after single value was emitted, because there wasn't a source stream to exhaust of values. But, luckily the weld function actually knows how to handle this situation and will simply close the output memory channel, once the pump function exits, if there is no input. So, this turns out to be safe.

I think there should be a test for this. Maybe:

async def test_insert_value_without_input():
    async with Pipeline.create(
        InsertValue('a')
    ) as pipeline, pipeline.tap() as aiter:
        results = [v async for v in aiter]
        assert results == ['a']

And maybe a tiny note should be added to the documentation specifying the expected behaviour for this corner case.

[mikenerone]

My original intended use case for InsertValue assumes that it will receive further values, in other words it was supposed to not be used as the first section and thus would always get an input. I was never really intending it to be used as first section.

I kinda figured, but the type annotation says it's optional, so either the no-input case should be handled or the type annotation should be fixed. The latter opens its own can of worms because we'd then be overriding refine() with an incompatible signature, which is another thing the type-checkers will rightfully complain about. It seemed to me that the former option was simply easier. And it's kindof a feature, as I can't think of a better way to make a single-value source in one line, which I can imagine there are uses for.

I think there should be a test for this. Maybe:

Heh, this PR already contains one. Funnily enough, our test implementations are identical, except for the name and the fact that I used "n" as my input value instead of "a", simply because that's the character the existing test_insert_value() uses. Edit: Ooo, and my extra indentation on the InsertValue() line. How embarrassing.

async def test_insert_value_no_input(autojump_clock):
    async with Pipeline.create(
            InsertValue('n')
    ) as pipeline, pipeline.tap() as aiter:
        results = [v async for v in aiter]
        assert results == ['n']

And maybe a tiny note should be added to the documentation specifying the expected behaviour for this corner case.

Good call. I'll add something.

[mikenerone]

@andersea Looking at the docs, I noticed that all other sections that can be used as starting sections say "can be used as a starting section, if a source is provided." Should we add support for a source parameter to InsertValue for consistency (though it would be optional in this case)? (I'll wait for your answer before making the doc update.)

[andersea]

@mikenerone Ok, I got through the review. Looks good. I haven't used the monkeypatch module, but I see how it helps a lot in this case. But I had to go through everything step by step to make sure I understood what was going on.

If you want to add an optional source parameter to InsertValue, go ahead. I am fine with it either way.

[mikenerone]

Sorry for the delay, I'll get to it eventually. :P

[mikenerone]

@andersea Ok, ready now. In the end I felt like the source parameter didn't really fit, so I just added the small doc update.

[andersea]

Thank you!