New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for Bug #877 Extended cue breakage in vtt files #1236

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@trmb
Contributor

trmb commented May 24, 2014

Only send actual time components to parseCueTime (no extended info). I also some commented out tests since they no longer apply.

@mmcc mmcc added the enhancement label May 28, 2014

@mmcc

This comment has been minimized.

Show comment
Hide comment
@mmcc

mmcc May 28, 2014

Member

Thanks for the PR! What exactly do you mean when you say the tests no longer apply? The point of those tests was that anyone submitting a fix for #877 wouldn't need to write them, so those definitely shouldn't be removed. If the PR is ready, those should be uncommented and passing.

Member

mmcc commented May 28, 2014

Thanks for the PR! What exactly do you mean when you say the tests no longer apply? The point of those tests was that anyone submitting a fix for #877 wouldn't need to write them, so those definitely shouldn't be removed. If the PR is ready, those should be uncommented and passing.

@mmcc mmcc added bug and removed bug labels May 28, 2014

@trmb

This comment has been minimized.

Show comment
Hide comment
@trmb

trmb May 28, 2014

Contributor

Hello! I removed them because my change was to prevent parseCueTime from receiving extra data in the date string. Right now those tests would still fail, though thinking on it, perhaps parseCueTime should still sanity check it's string before proceeding.

Contributor

trmb commented May 28, 2014

Hello! I removed them because my change was to prevent parseCueTime from receiving extra data in the date string. Right now those tests would still fail, though thinking on it, perhaps parseCueTime should still sanity check it's string before proceeding.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 28, 2014

Member

I think this is a fine way to handle it. At some point we need to actually store the extra flags that would come after the end time, but at the moment we don't do anything with them. And either way it's probably better to sanitize the end time before passing it to parseCueTime.

Any chance you want to squash #183 at the same time? And maybe write a test to test this new approach?

Member

heff commented May 28, 2014

I think this is a fine way to handle it. At some point we need to actually store the extra flags that would come after the end time, but at the moment we don't do anything with them. And either way it's probably better to sanitize the end time before passing it to parseCueTime.

Any chance you want to squash #183 at the same time? And maybe write a test to test this new approach?

When breaking the cue time line apart, split on
one or more tabs and/or spaces fixes #183
Uncommented tests, added new ones for multi and
mixed whitespace cases
@trmb

This comment has been minimized.

Show comment
Hide comment
@trmb

trmb May 29, 2014

Contributor

Sure! instead of splitting on just one space, now split on one or more tab or space (which appears to be covered in the spec). I added a test for multiple tabs / spaces and can add more if you can think of something that doesn't have coverage. Nothing was immediately popping out though, at least within what it can currently handle.

Contributor

trmb commented May 29, 2014

Sure! instead of splitting on just one space, now split on one or more tab or space (which appears to be covered in the spec). I added a test for multiple tabs / spaces and can add more if you can think of something that doesn't have coverage. Nothing was immediately popping out though, at least within what it can currently handle.

@heff heff closed this in bb50466 Jun 13, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jun 13, 2014

Member

Thanks again for this! Tests look good to me. I just merged it in and it will go out with v4.7.0.

Member

heff commented Jun 13, 2014

Thanks again for this! Tests look good to me. I just merged it in and it will go out with v4.7.0.

@trmb trmb deleted the trmb:feature/extended-cue-breakage-877 branch Oct 2, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment