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

Minor bug fixes #165

Merged
merged 2 commits into from Jul 24, 2022
Merged

Minor bug fixes #165

merged 2 commits into from Jul 24, 2022

Conversation

p3g4asus
Copy link
Collaborator

Hi,
with this PR I am fixing some minor bug I found in the code. They are:
Keep last value of progress and ETA when parsing fails
Fix cache option presence checking
When dumping json of a playlist, do not throw exception if the playlist has only some "erratic" videos and not all

Thanks for your work.

Fix cache option presence checking
When dumping json of a playlist, do not throw exception if the playlist has only some "erratic" videos and not all

if (exitCode > 0) {
if (exitCode > 0 && (!json || (json && out.isEmpty()))) {
Copy link
Owner

Choose a reason for hiding this comment

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

can this not be solved using --ignore-errors flag?
If not, maybe we should still throw exception if out is not a valid json.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see. I have to make some tests. About throwing an exception on invalid JSON, you are definitely right. I will try to make this part more robust.

Copy link
Collaborator Author

@p3g4asus p3g4asus Jul 13, 2022

Choose a reason for hiding this comment

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

I have made one more test. --ignore-errors does not prevent ytdlp returning 1 and not 0 when having a single erratic video in a playlist.
We could change the condition as follows

        if (exitCode > 0 && (!json || (json && (out.isEmpty() || !request.hasOption("--ignore-errors"))))) {

For json sanity checking we could use the code inside getInfo. Obviously we should try to parse the output only if the above condition fails and exitCode is > 0. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

This condition looks good. But maybe make it a little easier to read. Something like this with the remaining condition inside ignoreErrors function.
exitCode > 0 && !ignoreErrors(request, out)
We don't have to do the json sanity part because you added the --ignore-errors in the condition. That is good enough.

Copy link
Owner

@yausername yausername left a comment

Choose a reason for hiding this comment

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

Left a comment, but I'm approving the changes.
Thanks for the contribution!

…dable and ignores exit codes grater than 0 honoring the --ignore-errors input switch
@p3g4asus
Copy link
Collaborator Author

Now it should be OK.

@yausername
Copy link
Owner

Looks good. Feel free to merge.

@p3g4asus p3g4asus merged commit 01f35d7 into yausername:master Jul 24, 2022
@p3g4asus p3g4asus deleted the fixesPR branch August 7, 2022 05:37
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.

None yet

2 participants