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

Update lib/svtplay_dl/subtitle/__init__.py #1358

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bartdedecker
Copy link

Added lines 70 and 71: Sometimes, data is None and causes svtplay-dl to crash on line 78 in the method save_file.
The addition makes the code more robust.

Added lines 70 and 71: Sometimes, data is None and causes svtplay-dl to crash on line 78 in save_file.
@spaam
Copy link
Owner

spaam commented Apr 14, 2021

the commit message is not that great. we have several __init__.py files. it would be nice if you could fix that.

@bartdedecker bartdedecker changed the title Update __init__.py Update lib/svtplay_dl/subtitle/__init__.py Apr 14, 2021
@iwconfig
Copy link
Contributor

iwconfig commented Jun 4, 2021

Just a tip

Don't reference line numbers. Such references do easily break when code is added or removed. When you commit, it's not difficult to look at the diff and see what's changed. This information is available in PR as well.

Your commit message only really need the essential stuff. Also you should change the summary of the commit as well, not just the PR.

For example

Update lib/svtplay_dl/subtitle/__init__.py
Sometimes data is None which causes svtplay-dl to crash

or

Update lib/svtplay_dl/subtitle/__init__.py
Prevent crash when data is None

or

subtitle: return None if data is None
file_d.write shan't write None and svtplay-dl shan't crash!

or simply

subtitle: Don't crash if data is None

You might already know this but the first line is the commit summary (or "title") and the rest is the description. For trivial commits, you can skip the description if you want. You should follow the common 50/72 practice; limit the summary to no more than 50 characters, and each line of the description (the rest of the message) to no more than 72 characters. Please do prefer to add a blank line between summary and description, but with short descriptions such as this one I find it not really necessary.

You can change a commit message with git commit --amend. Since you've already pushed the old commit you have to force push the edited (new) commit. This replaces the old commit on your remote origin. To force push a commit, simply run git push -f.

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.

3 participants