iheartradio / open-m3u8

Open Source m3u8 Parser
Other
248 stars 94 forks source link

Support writing of playlist #7

Closed derjust closed 9 years ago

derjust commented 9 years ago

This is an implementation to write playlist based on the data structure present in memory.

For convenience it follows the same API structure of the parse methods. It uses the same handler approach for parsing and writing.

To do so, what was formally a Handler is technically now a Parser. Whereas there is also a Writer interface and a Handler nowadays is the superset providing Parsing and Writing. That way (theoretically) the library could support different stages of supported tags for parsing and writing.

I tried to implement the tags as close to the spec as possible. My testing set might be limited therefore I added the samples from the spec document for further and more intense test cases.

The other PR #6 (Support for lenient, #3) is applied on this changes in this branch: https://github.com/derjust/open-m3u8/commit/24666bb3e5034deb5a5fe4eaedac0cc6633b0adc

Wopple commented 9 years ago

I appreciate all the work, but I don't think I can accept this approach.

The main problem is that handlers should not be both parsers and writers. I see parsing and writing as two very different tasks and they should be kept separate in the code as well. We could rename handlers to parsers though to better distinguish the two. Ideally, the two processes would look something like this:

Parsing

// InputStream -> parse a Playlist -> validate semantics -> return Playlist

PlaylistParser parser = new PlaylistParser(inputStream, format, encoding);

try {
    Playlist playlist1 = parser.parse();
    Playlist playlist2 = parser.parse();
} catch (ParseException exception) {
    // something wrong with the syntax of the playlist
} catch (M3u8Exception exception) {
    // Playlist type was parsed, but there is a semantic error
}

Writing

// Playlist -> validate semantics -> write a Playlist to OutputStream

PlaylistWriter writer = new PlaylistWriter(outputStream, format, encoding);

try {
    writer.write(playlist1);
    writer.write(playlist2);
} catch (M3u8Exception exception) {
    // there is a semantic error
}

Currently on master, semantic validation is mixed together with parsing which is something I would like to change at some point. That is one good reason to not conflate parsing and writing. If I want to refactor the parsing code in the context of the changes in this PR, I would also have to refactor the writing code at the same time. Then maintaining either one of them will become more difficult. It would be better if they were built to be orthogonal to each other sharing only the semantic validation step.

If you would like to discuss this in more depth, you can find me on IRC.

server: freenode channel: #open-m3u8

derjust commented 9 years ago

For reference, the branch in question is https://github.com/derjust/open-m3u8/tree/writer_squash GitHub closed it due to some branch cleanup. Discussion remains upon - I guess :-)