Skip to content
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

add property to get webvtt content without needing a file #34

Merged
merged 1 commit into from
Nov 15, 2020

Conversation

DawoudSheraz
Copy link
Contributor

@DawoudSheraz DawoudSheraz commented Sep 22, 2020

Description

This PR adds a property content in WebVTT object to obtain the formatted vtt without the need to create a .vtt file on the system. This can be helpful in cases when the data needs to be saved on a remote server and not on the hosted server. The addition of property will remove the overhead of creating a temporary vtt file and then reading the content from it.

Unit Tests Result

image

@DawoudSheraz
Copy link
Contributor Author

@glut23 Hello. Can you take a look and provide your thoughts? Thanks

@DawoudSheraz
Copy link
Contributor Author

@glut23 Hi. Did you get a chance to look at this? Thank You

@DawoudSheraz
Copy link
Contributor Author

@glut23 Hello. Pinging to get an update. Thank You

@glut23
Copy link
Owner

glut23 commented Nov 3, 2020

Hey @DawoudSheraz apologies for not replying earlier. Having this is indeed useful and it would be good to add it in. I would use the class WebVTTWriter to do it. Also it would be great if you could have the docs also updated to reflect this new feature.

@DawoudSheraz
Copy link
Contributor Author

Hey @DawoudSheraz apologies for not replying earlier. Having this is indeed useful and it would be good to add it in. I would use the class WebVTTWriter to do it. Also it would be great if you could have the docs also updated to reflect this new feature.

Sure, I will update the PR and the docs and then follow-up. Thanks

webvtt/webvtt.py Outdated
This property is useful in cases where the webvtt content is needed
but no file saving on the system is required.
"""
output = "WEBVTT"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the class WebVTTWriter to do this.

Caption('00:00:00.500', '00:00:07.000', ['Caption test line 1', 'Caption test line 2']),
Caption('00:00:08.000', '00:00:15.000', ['Caption test line 3', 'Caption test line 4']),
]
expected_content = "WEBVTT" \
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use textwrap dedent for better readability?
`
textwrap.dedent("""
WEBVTT

00:00:00.500 --> 00:00:07.000
Caption test line 1
Caption test line 2

...
""").strip()
`

@DawoudSheraz DawoudSheraz requested a review from glut23 November 4, 2020 15:23
@DawoudSheraz
Copy link
Contributor Author

@glut23 Hello. I have updated the code and addressed the comments. Please take a second look. Thank You

f.writelines(['{}\n'.format(l) for l in c.lines])
f.write(self.webvtt_content(captions))

def webvtt_content(self, captions):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To do this better create a list and add each line to it, then use '\n'.join.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glut23 If each line were separated by a single \n, it would have made sense. But there is inconsistency in the newline after WEBVTT, identifier, and timestamps. Using a list would make it a bit difficult to read and understand because of the combination of strings with \n or empty strings during appending and the last join to get the desired results. Adding one variant of code as following:

        output = ["WEBVTT"]
        for caption in captions:
            if caption.identifier:
                output.append("\n{}".format(caption.identifier))
            output.append('\n{} --> {}'.format(caption.start, caption.end))
            for line in caption.lines:
                output.append(line)
        return '\n'.join(output)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @DawoudSheraz apologies for the late reply. That code makes the current tests to break. The \n handling in between elements is not very readable so I would just get rid of it. This is how I would do it:

    output = ['WEBVTT']
    for caption in captions:
        output.append('')
        if caption.identifier:
            output.append(caption.identifier)
        output.append('{} --> {}'.format(caption.start, caption.end))
        output.extend(caption.lines)
    return '\n'.join(output)

Also you test had an issue expecting a last blank line to be present in the resulting content. You can do this:

    expected_content = textwrap.dedent("""
            WEBVTT

            00:00:00.500 --> 00:00:07.000
            Caption test line 1
            Caption test line 2

            00:00:08.000 --> 00:00:15.000
            Caption test line 3
            Caption test line 4
            """).strip()

f.writelines(['{}\n'.format(l) for l in c.lines])
f.write(self.webvtt_content(captions))

def webvtt_content(self, captions):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @DawoudSheraz apologies for the late reply. That code makes the current tests to break. The \n handling in between elements is not very readable so I would just get rid of it. This is how I would do it:

    output = ['WEBVTT']
    for caption in captions:
        output.append('')
        if caption.identifier:
            output.append(caption.identifier)
        output.append('{} --> {}'.format(caption.start, caption.end))
        output.extend(caption.lines)
    return '\n'.join(output)

Also you test had an issue expecting a last blank line to be present in the resulting content. You can do this:

    expected_content = textwrap.dedent("""
            WEBVTT

            00:00:00.500 --> 00:00:07.000
            Caption test line 1
            Caption test line 2

            00:00:08.000 --> 00:00:15.000
            Caption test line 3
            Caption test line 4
            """).strip()

@DawoudSheraz DawoudSheraz force-pushed the dsheraz/webvtt-content branch from ee5904a to a0417d8 Compare November 13, 2020 15:21
@DawoudSheraz
Copy link
Contributor Author

@glut23 Thanks for the feedback. I have updated the code.

@glut23 glut23 merged commit 2f01107 into glut23:master Nov 15, 2020
@glut23
Copy link
Owner

glut23 commented Nov 15, 2020

Hey @DawoudSheraz I merged the code and I will prepare a release as soon as I can. Thanks for contributing!

@DawoudSheraz
Copy link
Contributor Author

Hey @DawoudSheraz I merged the code and I will prepare a release as soon as I can. Thanks for contributing!

Glad to hear that, thank you.

@glut23
Copy link
Owner

glut23 commented Nov 18, 2020

Hey @DawoudSheraz 0.4.6 is now released. Thanks.

@DawoudSheraz
Copy link
Contributor Author

Awesome, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants