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 NapiProjekt provider #489

Merged
merged 1 commit into from
Aug 22, 2015
Merged

Add NapiProjekt provider #489

merged 1 commit into from
Aug 22, 2015

Conversation

bogdal
Copy link
Contributor

@bogdal bogdal commented Aug 18, 2015

This PR adds NapiProjekt provider.

Fixes #384

@Diaoul
Copy link
Owner

Diaoul commented Aug 21, 2015

This looks like a very complete PR. I'll review soon, thanks for your work.

@Diaoul Diaoul self-assigned this Aug 21, 2015
Diaoul added a commit that referenced this pull request Aug 22, 2015
@Diaoul Diaoul merged commit d7ecc58 into Diaoul:master Aug 22, 2015
@radekbenkel
Copy link

@bogdal Thank you for that PR! 🍺 Wanted to do this myself - good that I checked PR section first;)

@Diaoul I see that it's merged to master, but I don't see those changes in latest 1.0.1 release - was this removed after all?

@Diaoul
Copy link
Owner

Diaoul commented Aug 27, 2015

It was merged into master by error. I merged it to develop instead.

@castortray
Copy link

when we can expect that napiprojekt.py provider will be added to master release ?

@Diaoul
Copy link
Owner

Diaoul commented Oct 29, 2015

@bogdal: the provider doesn't seem to work anymore. Unittests are failing: https://travis-ci.org/Diaoul/subliminal/jobs/88170450#L395

@bogdal
Copy link
Contributor Author

bogdal commented Oct 30, 2015

@Diaoul ok, I will check.

@bogdal bogdal mentioned this pull request Oct 30, 2015
@bogdal
Copy link
Contributor Author

bogdal commented Oct 30, 2015

@MichalE7 It looks like napiprojekt gives subtitles in a different format than srt. I assume that we should convert them on the provider level.

@MichalE7
Copy link

@bogdal on provider level it means where and who can do it? Sickrage, subliminal or napiprojekt itself?
Here is the screenshot of napiprojekt standalone application settings.

napiprojekt_opcje

Based on wikipedia :) srt subtitles should look like the ones downloaded from napiprojekt standalone app: https://en.wikipedia.org/wiki/SubRip

@Diaoul
Copy link
Owner

Diaoul commented Oct 30, 2015

The format you downloaded with subliminal looks like MicroDVD. Not sure why napiprojekt doesn't return consistent subtitle format, this should be handled by subliminal.

@MichalE7
Copy link

MichalE7 commented Nov 2, 2015

I did some more checking.
Default setting for standalone app is to download subtitles in 'Original' format so I suppose it downloads subtitles in the format that was uploaded to the database without any conversion.
In settings you can chose to download subtitles in SRT format which are probably converted from the original. I guess conversion is missing in the script.

Additionally while browsing sites I came across dragon666 project called napi which includes a script to detect and convert subtitles format.
It can be found here: https://github.com/dagon666/napi

@Diaoul
Copy link
Owner

Diaoul commented Nov 3, 2015

Subliminal can detect a valid/invalid subtitle file with

if not subtitle.is_valid():

It should be done when using the CLI, I wonder why it didn't happen here.

I plan to develop a subtitle file format converter for any kind of format with a special focus on TTML for features. For now I think the best is to just discard them somehow and check why the result did get past the is_valid test with proper unittests.

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

Successfully merging this pull request may close these issues.

5 participants