Skip to content

[IMPROVEMENT] Don't add files with empty filename & show better msg for multiple input files.#994

Merged
cfsmp3 merged 1 commit into
CCExtractor:masterfrom
saurabhshri:blank-inp-filenames
Dec 18, 2018
Merged

[IMPROVEMENT] Don't add files with empty filename & show better msg for multiple input files.#994
cfsmp3 merged 1 commit into
CCExtractor:masterfrom
saurabhshri:blank-inp-filenames

Conversation

@saurabhshri

Copy link
Copy Markdown
Member

In raising this pull request, I confirm the following (please check boxes):

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

My familiarity with the project is as follows (check one):

  • I have never used CCExtractor.
  • I have used CCExtractor just a couple of times.
  • I absolutely love CCExtractor, but have not contributed previously.
  • I am an active contributor to CCExtractor.

If CCExtractor is passed an empty argument, it tries to open it as a file. E.g.
ccextractor "" OR
ccextractor {file1} "" {file2}

In case of multiple input files, if one or more failed to open, CCExtractor exists with the message Failed to open input file: File does not exist. Since my second parameter was an empty filename "" (came from an empty key in a script), I kept on thinking somehow "," is being appended to filename.
It showed : Input: filename,.

With this PR:

  • If filename is empty, that file is skipped.
  • If one or more file fails to open, it tells that.
  • Shows number of files while dumping parameters.

@ccextractor-bot

ccextractor-bot commented Jul 17, 2018

Copy link
Copy Markdown
Collaborator

CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results:

Report Name Tests Passed
Broken 13/13
DVB 4/7
DVR-MS 2/2
General 27/27
Hauppage 3/3
MP4 3/3
NoCC 10/10
Teletext 21/21
WTV 13/13
XDS 34/34
CEA-708 14/14
DVD 3/3
Options 86/86

It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you).

Your PR breaks these cases:


Check the result page for more info.

Comment thread src/ccextractor.c Outdated
{
case -1*ENOENT:
fatal(EXIT_NO_INPUT_FILES, "Failed to open input file: File does not exist.");
fatal(EXIT_NO_INPUT_FILES, "Failed to open one or more input file(s): File does not exist.");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this accurate? It doesn't seem possible that we can fail to open more than once since we are fatal()'ing on the first failure.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You're right, I am sorry. Updated to Failed to open one of the input file(s): File does not exist.

Comment thread src/lib_ccx/params_dump.c
mprint("Files (%d): ", ctx->num_input_files);
for (int i=0;i<ctx->num_input_files;i++)
mprint ("%s%s",ctx->inputfile[i],i==(ctx->num_input_files-1)?"":",");
mprint ("%s%s",ctx->inputfile[i],i==(ctx->num_input_files-1)?"":", ");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems more debug that general, maybe mprint() is too much?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What would you suggest? :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do we use for debug messages? :-)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@cfsmp3 I changed this (https://github.com/CCExtractor/ccextractor/pull/994/files#diff-20a92dffa6342caa6a83fd990812e4cbR14) to debug message. Should I change the current line (L16) as well? It was already in mprint though, and I think having the ability to see the files being processed is fine without running it in debug mode.

@ccextractor-bot

ccextractor-bot commented Jul 17, 2018

Copy link
Copy Markdown
Collaborator

CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results:

Report Name Tests Passed
Broken 12/13
DVB 4/7
DVR-MS 2/2
General 27/27
Hauppage 3/3
MP4 3/3
NoCC 10/10
Teletext 21/21
WTV 13/13
XDS 34/34
CEA-708 14/14
DVD 3/3
Options 85/86

It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you).

Your PR breaks these cases:


Check the result page for more info.

@saurabhshri saurabhshri force-pushed the blank-inp-filenames branch 2 times, most recently from 22b74e1 to 71ce5b9 Compare July 18, 2018 08:27
@thealphadollar

Copy link
Copy Markdown
Contributor

@saurabhshri Ping.

@saurabhshri

Copy link
Copy Markdown
Member Author

@thealphadollar @cfsmp3 recommended a change in the PR. I am waiting for his reply! :)

@cfsmp3

cfsmp3 commented Sep 21, 2018

Copy link
Copy Markdown
Contributor

@saurabhshri don't use mprint :-) If you want to write debug messages, use dbg_print instead.

@saurabhshri

Copy link
Copy Markdown
Member Author

@cfsmp3 Oh, I misunderstood earlier! Done. :)

@thealphadollar

Copy link
Copy Markdown
Contributor

@cfsmp3 can we merge this?

@cfsmp3 cfsmp3 merged commit 9c20e0a into CCExtractor:master Dec 18, 2018
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.

4 participants