Adding test_parse_file_with_missing_index_on_first_line

[Lucas-C]

My original problem had already been spotted in #51:
some .srt files I found do not contain any initial index for the first block.

Hence I started working on adding support to the srt lib for parsing those files,
but I realized that it works totally fine with srt.parse(subs, ignore_errors=True).

Hence this PR only adds an explicit unit test,
to make srt behaviour for those kind of files explicit.

[cdown]

Thanks! The concept is fine, but please use a Hypothesis test instead of using a file fixture :-)

See test_can_parse_index_trailing_ws and the like for an example of how to do that. In your case, you want to compose some subtitles and remove the first line.

[cdown]

I'm also up for adding support to parse SRT files without an index, by the way. Those subtitles can get index=None, which can then be handled by to_srt() and the comparison checks.

[Lucas-C]

I adapted the test to use hypothesis.
I find the resulting unit test a lot less readable, but I also see the point of using hypothesis to strengthen the tests.

I'm also up for adding support to parse SRT files without an index, by the way

I'll let you handle that 😊

[cdown]

Thanks! I see you pushed a new commit. But I guess I was asking why do the loop over input_subs at all? Usually we only need to do that when we need to mutate Subtitle object content. Do we handle the case where there's a single errant sub differently from if there's a trailing valid one?

[cdown]

To be clear, this is what I'm wondering about (untested):

@given(st.lists(subtitles(), min_size=1))
def test_parse_file_with_missing_index_on_first_line(input_subs):  # cf. issue #51
    block = srt.compose(input_subs)
    block = block[block.find("\n") + 1:]
    with pytest.raises(srt.SRTParseError):
	list(srt.parse(block))
    assert list(srt.parse(block, ignore_errors=True)) == input_subs[1:]

[Lucas-C]

I adopted the version of the code that you suggested.

[cdown]

Sorry for another round of review, but I'm confused again. Why if block instead of using min_length=1? Why len() check instead of just comparing the lists directly?

I promise we're getting somewhere with this PR, I'm just confused at the changes from the suggested code :-)

[Lucas-C]

Why if block instead of using min_length=1?

I am simply not very familiar with hypothesis API / features :)
I switched to using min_size=1

Why len() check instead of just comparing the lists directly?

Because the Subtitle.index change in some cases.
I'll let you test it if you want proof ;)

[cdown]

Because the Subtitle.index change in some cases.

Ah, for that, pass reindex=False to srt.compose.

[Lucas-C]

Ok, done!

[cdown]

Looks like tests failed because black format check failed. Want me to just rebase and fix up? Otherwise feel free to run black on tests/test_srt.py and squash into one commit. :-)

https://github.com/cdown/srt/runs/5382080756?check_suite_focus=true

[Lucas-C]

I prettified the code with black and squashed my commits.
Note that squashing can be done through GitHub by maintainers when they merge a PR.

[cdown]

Yeah sure, I just wasn't sure if you'd prefer I just blacken the code and fix it up myself without your intervention (although that won't show as "merged" on GitHub UI). I'll run CI and merge. Thanks!

[coveralls]

Coverage remained the same at 100.0% when pulling c3b5ed9 on Lucas-C:test_parse_file_with_missing_index_on_first_line into e518e28 on cdown:develop.

[cdown]

Thanks again!

[cdown]

And thanks again for bringing this general case up, 745d5ee now supports it officially.