search

IRT-Open-Source / scf

Subtitling Conversion Framework
Apache License 2.0
52 stars 18 forks source link

Resolve issue #13 #14

Open nigelmegitt opened 8 years ago

nigelmegitt commented 8 years ago

When the application of a time offset would result in a negative begin or end time, rather than terminating with an error, prune the p or span element that would have that attribute, and issue a warning.

Also includes some tidying of duplicated code where begin and end attributes can be handled with a single template.

andreastai commented 8 years ago

See comment on #13. This is indeed an issue that needs to be dealt with. I propose that we first fix the requirement.

nigelmegitt commented 6 years ago

@tairt is there any action for me on this pull request now, or is it not worth it until the requirement is fixed? I see there are conflicts, but I haven't looked at resolving them yet, because I'm not sure if it is worth it.

andreastai commented 6 years ago

@nigelmegitt I am sorry...in the end of the year hectic I have overseen this. I will discuss this internally and come back to you asap.

nigelmegitt commented 6 years ago

@tairt And likewise I have only just noticed your comment from 10 days ago! Let me know when you have discussed internally.

spoeschel commented 6 years ago

After we discussed the issue and general SCF aspects, we plan to proceed as follows:

As we don't yet have a contribution guideline for the SCF, we will prepare one and add it to the repo. If this guideline will be fine for you, we will be able to continue here.

As discussed in #13, we would like to keep the new behaviour (discard content with resulting negative timestamps and output a warning) optional i.e. add a parameter to the XSLT to enable it. It seems to make sense to add the feature itself and the parameter in the same step. Would it be acceptable for you to adjust your contribution accordingly (in addition to rebase it onto the current release)?

We then would add a new feature branch (based on master) on which all the work for this feature will happen and for which you could open a merge request then. This way we can also extend the requirements PDF, and update README and changelog for the resulting new SCF release, before merging all the changes back to master as a whole.

Is this proceeding OK for you?

nigelmegitt commented 6 years ago

Hi @spoeschel yes, that sounds fine.

spoeschel commented 6 years ago

We are still working on it....meanwhile it also seems that we will have to ask you/the BBC to sign a CLA, as this is not a trivial contribution...

nigelmegitt commented 6 years ago

Ping! I don't think I've received a CLA to review and hopefully sign yet.

andreastai commented 6 years ago

@nigelmegitt I want to apologize for the delay in handling the PR and adding a contribution guideline. As you can imagine this CLA may have to be discussed with our other projects and set as a general guideline. This takes more time as we thought. We hope to have it soon. Thanks you for your patience and for leaving this PR open.