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

[IMPROVEMENT] Move dependencies to a third party directory #1219

Merged
merged 3 commits into from Jan 30, 2020

Conversation

@NilsIrl
Copy link
Contributor

NilsIrl commented Jan 28, 2020

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 am an active contributor to CCExtractor.

Move thirdparty dependencies to /src/thirdparty and remove redundant code in linux/builddebug and linux/build_hardsubx.

Add .clang-format and github action to check if the code is formatted.

Why?

  • Allows to easily differentiate between dependencies and ccextractor code:
    • Makes it a lot less confusing to new contributors as to why there are files everywhere.
    • Tools can leverage this to only act on some files (e.g. clang-format)
@NilsIrl NilsIrl force-pushed the NilsIrl:format branch 3 times, most recently from cf305c2 to 0a2f922 Jan 28, 2020
@NilsIrl NilsIrl changed the title [IMPROVEMENT] Move dependencies in a folder [IMPROVEMENT] Move dependencies in a third party directory Jan 28, 2020
@NilsIrl NilsIrl changed the title [IMPROVEMENT] Move dependencies in a third party directory [IMPROVEMENT] Move dependencies to a third party directory Jan 28, 2020
@NilsIrl NilsIrl force-pushed the NilsIrl:format branch from 0a2f922 to 03ddbb9 Jan 28, 2020
@ccextractor-bot

This comment has been minimized.

Copy link
Collaborator

ccextractor-bot commented Jan 28, 2020

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

Report Name Tests Passed
Broken 11/13
DVB 4/7
DVR-MS 0/2
General 26/27
Hauppage 3/3
MP4 3/3
NoCC 10/10
Teletext 20/21
WTV 10/13
XDS 28/34
CEA-708 14/14
DVD 3/3
Options 34/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.
@NilsIrl NilsIrl force-pushed the NilsIrl:format branch 4 times, most recently from 5b9473d to e936e13 Jan 28, 2020
@NilsIrl NilsIrl marked this pull request as ready for review Jan 29, 2020
@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Jan 29, 2020

I've also added a clang-format GitHub action that checks if the code is formatted correctly. It currently fails because the code isn't formatted.

To format the code the following command has to be ran:

find src/ -type f -name '*.c' -name '*.c' | xargs clang-format -i

If you want I can add a commit with the formatted code. Otherwise, this can be merged and once it is merged, a commit with the code formatted should be pushed (so that the GitHub action passes).

@NilsIrl NilsIrl requested a review from cfsmp3 Jan 29, 2020
@ccextractor-bot

This comment has been minimized.

Copy link
Collaborator

ccextractor-bot commented Jan 29, 2020

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

Report Name Tests Passed
Broken 11/13
DVB 3/7
DVR-MS 0/2
General 26/27
Hauppage 3/3
MP4 3/3
NoCC 10/10
Teletext 14/21
WTV 10/13
XDS 28/34
CEA-708 14/14
DVD 3/3
Options 31/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.
@cfsmp3 cfsmp3 requested a review from canihavesomecoffee Jan 29, 2020
@NilsIrl NilsIrl force-pushed the NilsIrl:format branch from e936e13 to 903026e Jan 29, 2020
@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Jan 29, 2020

To have an idea of what the current .clang-format gives I have a branch which clang-format has been run on which branches from this branch:

master...NilsIrl:formatted

Please tell me if something about the format is wrong/doesn't fit the current code style.

This branch can also be merged after.

@cfsmp3

This comment has been minimized.

Copy link
Contributor

cfsmp3 commented Jan 29, 2020

@NilsIrl NilsIrl force-pushed the NilsIrl:format branch from 903026e to 002551f Jan 29, 2020
@canihavesomecoffee

This comment has been minimized.

Copy link
Member

canihavesomecoffee commented Jan 30, 2020

@NilsIrl I'd rather like to see that new GitHub Action for formatting to be in a separate PR. Makes it easier to merge and keep them separated.

@canihavesomecoffee

This comment has been minimized.

Copy link
Member

canihavesomecoffee commented Jan 30, 2020

Once the action is gone (moved to separate PR), I think we can merge this one already.

@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Jan 30, 2020

@NilsIrl I'd rather like to see that new GitHub Action for formatting to be in a separate PR. Makes it easier to merge and keep them separated.

Should the addition of .clang-format be moved to another PR as well? If so, should it be in the same PR as the GitHub action PR?

@canihavesomecoffee

This comment has been minimized.

Copy link
Member

canihavesomecoffee commented Jan 30, 2020

@NilsIrl I'd rather like to see that new GitHub Action for formatting to be in a separate PR. Makes it easier to merge and keep them separated.

Should the addition of .clang-format be moved to another PR as well? If so, should it be in the same PR as the GitHub action PR?

Yes, I'd do that in the same PR where you introduce the action, as they are related :)

@NilsIrl NilsIrl force-pushed the NilsIrl:format branch from 002551f to 8158ebe Jan 30, 2020
@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Jan 30, 2020

Done

@NilsIrl NilsIrl mentioned this pull request Jan 30, 2020
5 of 5 tasks complete
@cfsmp3 cfsmp3 merged commit af6d828 into CCExtractor:master Jan 30, 2020
7 of 9 checks passed
7 of 9 checks passed
build_shell
Details
build
Details
build_autoconf
Details
cmake
Details
cmake_ocr_hardsubx
Details
CI - linux Not all tests completed successfully, please check
Details
CI - windows Not all tests completed successfully, please check
Details
WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@NilsIrl NilsIrl deleted the NilsIrl:format branch Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.