Adding support for WebVTT (Web Video Text Tracks) (.vtt) format

[dsavinov-actionengine]

Problem and/or solution

Adding parsing and compiling for WebVTT (Web Video Text Tracks) (.vtt) format

How to test

1. Running unit-tests

/openformats/tests/formats/vtt/test_vtt.py contains tests for vtt
Use pytest openformats/tests/formats/vtt/test_vtt.py to run tests

2. Through testbed

Use "VTT" handler in the testbed

Reviewer checklist

Code:

• Change is covered by unit-tests
• Code is well documented, well styled and is following best practices
• Performance issues have been taken under consideration
• Errors and other edge-cases are handled properly

PR:

• Problem and/or solution are well-explained
• Commits have been squashed so that each one has a clear purpose
• Commits have a proper commit message according to TEM

[dsavinov-actionengine]

@kbairak please review

[kbairak]

Overall it looks good. I will take a more thorough look and invoke it through a debugger. I have a feeling some things can be simplified but I will have to experiment a bit. For the time being, just the one comment.

[kbairak]

It's good practice to include an order with each OpenString. This is somewhat easy to do:

from itertools import count

order = count()
for ... in ...:
    string = OpenString(..., order=next(order))

This will ensure each string will get an auto-incrementing value for order.

[dsavinov-actionengine]

Done.

[kbairak]

Great work! I think that only the first comment requires a change (the one with the multiple occurrences of -->). The rest are optional.

[kbairak]

Minor because it is not going to affect performance by a lot, but we could break here.

Actually, we should break because the --> part could be in an actual subtitle and we want to consider the first occurrence as the timing.

[dsavinov-actionengine]

Done

[kbairak]

This is part of the standard library. It might be more well-suited. https://docs.python.org/3/library/datetime.html#strftime-strptime-behavior

[dsavinov-actionengine]

This function is written based on a similar function in SRT handler. It takes a string as input and returns a string too. The built-in strftime()/strptime() are less convenient here. Let's leave this function as it is?

[kbairak]

There is a small probability (and if we're honest, transifex will probably stop the compilation process before the interpreter gets here) that this will raise a StopIteration. Maybe a try/except that raises a ParseError("stringset cannot be empty") would fit here.

[dsavinov-actionengine]

Done

[kbairak]

How about:

Suggested change

	hash_position = -1
	if subtitle_section.count('-->') > 0:
	arrow_pos = subtitle_section.index('-->')
	end_of_timings = subtitle_section.index('\n', arrow_pos + len('-->'))
	hash_position = end_of_timings + 1
	try:
	arrow_pos = subtitle_section.index('-->')
	except ValueError:
	hash_position = -1
	else:
	end_of_timings = subtitle_section.index('\n', arrow_pos + len('-->'))
	hash_position = end_of_timings + 1

I know mine is longer and this is a styling preference so feel free to ignore but I feel it is more pythonic.

[dsavinov-actionengine]

In current PR code, .index('-->') cannot produce an exception because it is under condition (.count('-->') > 0) in the previous line.
But .index('\n', arrow_pos + len('-->')) (in both current code and in your suggestion) can give an exception.
So in the new commit there is a rework.

[kbairak]

Could this line raise a ValueError?

[dsavinov-actionengine]

In theory, it could.
Absence of '\n' after timing means that subtitle text is missing. In such case, parser shall raise an exception earlier (function _parse_section(), line 104). Nevertheless, new commit adds ValueError handling here in compile() function, too.

[kbairak]

Great work!