I have already agreed that I need to break up the patches and be more clear.
But that has nothing to do with whether the parser should be allowed to output non-standard compliant cues. Nor does it have anything to do with if there should be one set of tests or two. On Mon, Feb 4, 2013 at 10:46 PM, Caitlin Potter < [email protected]> wrote: > We agree that there are errors in these tests (however I did not see that > one mentioned in your list of changes, which is a good reason to make much > smaller, more focused issues rather than massive pull requests) > > But your changes have done a lot more than just this. You've removed tests > claiming that they're duplicates without demonstrating that they're > duplicates (eg, claiming that "4 digits" and "greater than 999" are the > same thing, with the same code to step through, when they are in fact not). > But even if you do demonstrate that the code is the same, for things like > that I think it's a good idea to keep those tests in place for now, because > they're not actually the same thing. > > Removing tests, changing the expected number of cues (sometimes correct, > other times not so much), removing the comments regarding the syntax rules > (which are essentially a guide through the parser code) and replacing them > with the "relevant" section from the parser spec... Things like this, I > don't agree with. There are a huge number of changed files in your pull > requests, I haven't been over all of them, but a few of these things have > stood out. > > It will be a lot simpler if we can avoid these massive patches (I'm guilty > of this too) so that it's easier for other people to provide an input on > exactly what is correct and what isn't. > > But nevermind the "big patch" issue for now, the thing is that we all > agree that tests for the rules outlined in the "parsing" section are > needed. The issue is that some of us believe these tests are different from > the other tests (in that they will require a different FileParser > implementation). They can still use the same data files, and even sit in > the same folder. But some code changes are really necessary to make them > test the things you want them to test. > ________________________________________ > From: > dev-media-bounces+caitlin.potter=senecacollege...@lists.mozilla.org[dev-media-bounces+caitlin.potter= > [email protected]] on behalf of Kyle Barnhart [ > [email protected]] > Sent: Monday, February 04, 2013 10:21 PM > To: Ralph Giles > Cc: [email protected] > Subject: Re: WebVTT Parser Standards Compliance > > There are tests for every valid and invalid input we could think of. Let me > show an example of the changes I've made. > > 00:01.780 --> 00:02.300 > > That is a timing statement for a cue. The whitespace between the timestamp > and the arrow is required by the syntax rules but not required by the > parsing rules. So for tests where the whitespace is missing, I've changed > the expected number of cues from 0 to 1. > > 00:02.0005 > > The milliseconds are only allowed to have 3 digits. In the current tests > change it 00:02.005. This is not allowed by either syntax or parsing rules, > and by parsing rules the cue should be discarded. So I changed expected > cues from 1 to 0. > > These are only two of the many changes that had to be made to make the > tests correct according to the parsing rules. The other big change I made > is to reference the parsing rules being tested in the comments instead of > the syntax rules, since the syntax rules don't apply to a parser and the > parsing rules do. Otherwise I've made no changes to the intent of any test, > and I have added many missing tests, and removed duplicate test. I have not > removed any debug error checks and have added many missing ones. In all the > modified tests are more thorough and make sure the parser is correctly > outputs cues that are standards compliant. > > What Caitlin is now arguing is that the parsing library should have two > settings, one outputs non-standard cues, and one outputs standard cues, and > there is a set of tests for each. However I can see no possible reason ever > to output non-standard cues. In fact is it bad, dangerous, and a whole lot > of unnecessary work. The purpose of a standard is to make sure WebVTT > behaves the same in all settings. Outputting non-compliant cues directly > violates the standard. Allowing it can only serve to make it more difficult > to work with WebVTT, and developers will not know how their file will > behave from one application to the next. Therefore it is far less work and > far better to have one set of standard compliant tests, and fix the parser > to those standards. And it is better to do it now, then to go back and > re-engineer the thing later. > > Thanks, > Kyle Barnhart > > > On Mon, Feb 4, 2013 at 8:53 PM, Ralph Giles <[email protected]> wrote: > > > On 13-02-04 5:20 PM, Caitlin Potter wrote: > > > > > The issue here is that Kyle insists on rewriting unit tests that are > > concerned with the "syntax specification", rather than adding new tests > > that are concerned with the "parser specification". > > > > Like Chris, I'm a confused what the contended issue is. To be clear, are > > we looking at the version of the webvtt spec at > > https://dev.w3.org/html5/webvtt/ ? > > > > This has several sections which seem relevant: > > > > * 3.1 "Syntax" described the WebVTT text format and includes a few > > parser descriptions, such as the one for timestamps. > > > > * 3.2 "Parsing" describes the top-level parsing algorithm for WebVTT > > text files. > > > > * 3.3 "Cue text parsing" describes a parsing algorithm for the contents > > of each cue, building a particular dom-ish data structure. > > > > Is one of these sections the "syntax specification" and another the > > "parser specification"? If so, where do they disagree? Can you give a > > specific example where one is more permissive or restrictive than > another? > > > > The point of having a specific parser algorithm documented in the spec > > is to achieve uniform implementation. If everyone implements the > > algorithm (or its equivalent) the handling of edge cases will be more > > consistent than if everyone implements a parser from scratch based on > > some incomplete syntax description. So we should be implementing the > > parser the spec describes. If the spec is internally inconsistent we > > should file spec bugs and get the text fixed. > > > > Nevertheless, the current code doesn't pass--or even run to completion > > on--a number of the current tests, so it's difficult to tell what works > > and what doesn't. I think fixing that should be the highest priority for > > those working on the parser. Without tests we don't know where we stand, > or > > > > Kyle, Caitlin's suggestion that you provide a separate set of parser > > tests seems reasonable to me if she wants the current set for code > > coverage or additional features. The test sets can always be merged > > later if there's consensus that's appropriate. In the meantime you won't > > get in each other's way. > > > > - Ralph > > _______________________________________________ > > dev-media mailing list > > [email protected] > > https://lists.mozilla.org/listinfo/dev-media > > > _______________________________________________ > dev-media mailing list > [email protected] > https://lists.mozilla.org/listinfo/dev-media > _______________________________________________ dev-media mailing list [email protected] https://lists.mozilla.org/listinfo/dev-media
