[vlc-devel] [PATCH] Lua-SD: Adding a customized way to create Jamendo playlists for everyone to try

[chrcnt7 at swift-mail.com]

As advised by refp, I’ll answer to the list instead of fighting it out with him in IRC. ;)
I used stuff in brackets to malleate the quotes into something that is easier to handle.


> [===] Excessive comments [===]
> [Stripped massive quote]
> Comments are always nice, though if they are too verbose they
> certainly do more harm than good. I understand that this is a work-in-
> progress, but if this is to be merged I’d vote that the comments
> should be kept to a necessary minimum.
> If you’d like to explain the rationale(s) behind your
> implementation, do so on the mailing list and not directly within
> the implementation itself.
> The above applies to several sections within your implementation.
In fact, I’ve found it to be the most frustrating and aggravating experience having to collect important implementation details and remarks piecemeal from what can be a dozen – or even more – different websites, subpages, cross-links, wiki entries, vendor sites, random comments on chat that might have been archived properly or not or, as you even propose, mailing list archives. Think of it this way: Only guys that are _currently_ interested in the development of VLC would subscribe to the ML. There might be many stale subscriptions, but people who let their subscription go stale don’t care. And that’s not even a reversible relation! Only the subset of people who are interested in development _and_ have subscribed are even likely to read the mail here. (This excludes guys like me, when I just hanged out [hung out?] in the IRC channel to grab a few words of wisdom, to give just one example.) More importantly, it also won’t include any _future_ developers, as they will only get _new_ mail.
So if I were to do that, every poor guy who wanted to know more about the implementation would have to hunt down the mails from the list, possible read through the whole convo to be able to piece everything together and might still lack some insights after that. He should be eternally grateful for an experience this intense, but to me it feels like the only intense feeling he might get is ire towards the guys who thought the ML was an appropriate archive for that. :P Sorry, I won’t change that for now; I wrote all of this stuff for a reason.
(Granted, I could probably still reword or shorten it to make it better.)


> [===] VLC is not a documentation aggregator [===]
> [Stripped another massive quotation]
> If you want to make sure that people understand the syntax of the
> incoming data, including a link to the API reference is far more
> suitable than including a full example-response.
> It is not the responsibility of code within VLC to document an
> external API, especially when the documentation associated with the
> Jamendo API is good enough to answer any questions regarding it.
This is another point where I’ll have to disagree; adding to what I wrote before, I always thought the data structure and flow diagrams I found in LKML patchsets to be nice additions and sometimes even the pinnacle of ASCII-illustrations. ( ;) )
While I probably cannot assert this for my API doc, it’s always nice to have a representation of the API _as the actual programmer of the script understood it_ included, especially in case any discrepancies between the API and the code arise. Without such comments, one could never tell whether this was some oversight in the code or consciously written code with a false impression of the API in mind. It also helps to explain things in general.
Here, too, I strongly favor keeping this (as you might already have expected), although I’ll probably reword things to be clearer or to state them more concisely.

> [===] Auto-generated implementations are not too friendly [===]
> [Third quote killed]
> The above line is *2373* characters long, even for auto-generated
> content it is not too pleasant to look at, nor interact with.
> As stated in #videolan at freenode, I strongly feel like your approach
> to parsing the data is too complex to be a viable solution to the
> problem you have described.
> If you are trying to write an implementation that is as fast as
> possible, and this being your rationale; include a benchmark showing
> why your implementation is more suitable than a far less complex
> alternative using existing tools that can be used to parse *json*,
> *xml*, etc.
For the record (I think I already told you over IRC, but others might not know): I wrote this implementations for many weak reasons combined. I was fed up with XML and the problems parsing it, I wanted to see how fast I could make it go (but I should have benched primitives first – what I relied on to be fast, because those were the lowest level things I could find and I thought they had to be fast, turned out pretty much mediocre. In fact, Lua has some weirdly shaped tools I found somewhat useful for parsing, but I found to be constantly lacking the right ones and all options to be flawed in one way or another. I also wanted to experiment with stuff, especially with a special and very different approach on parsing.
I agree it’s probably not the _best_ implementation one could have written under other circumstances (for whatever one might think about when hearing this word), but I hold that it’s not bad either. Especially with the new changes, it should be quite quick. (Probably not beating C implementations of JSON parsers; but I wouldn’t be surprised at all if it beat DKJSON and did that with a comfortable margin, seeing as that is a Lua-only lib (when used without LPeg, as would be the case within VLC) that has to handle generic JSON)
Maybe I should stress again that the intended usecase is legacy support for all versions of VLC which can’t use the XML version. (Again, not that I’d like XML at all, but VLC already had a capable and nice binary code XML-lib when I started, so I chose to rely on that, as pretty much every other SD script.)


> - What has been used to generate the code responsible for parsing? -
>   If a bug is discovered, how do you recommend fixing it without
>   access to the code-generation?
>
> Do not be afraid to use whitespace
I don’t like my code broken up over many lines and split into many small insulas, as other people seem to prefer it. I found the splits to be quite arbitrary at times, which rather adds to confusion for me and makes it harder to read.

> [*snap*]
> The above is not auto-generated, but it is nonetheless not very
> readable; introducing some white-space certainly would not hurt the
> implementation.
>
> Refactoring to make things more readable
>From that quote, I wonder whether you have display problems, maybe!? There were whitespaces before every lollipop comment, to give just one example, and to give some other, the 'log(<...>)' statements, the 'local' declaration and the 'for'-loop header were on their own lines. If your representation differs, please fix it or tell me what I messed up. ;)

> [Removed another wall of text; good thing that FastMail collapses long
> quotes automatically in the message display ;) ]
> An array with suitable content and a loop would make the above so much
> less verbose.
Right, but if the initial repulsion can be overcome, it’s actually not that bad. I’m not as set on that issue as I’m on the others as I can really see how the code would look much nicer with it rewritten to use table-arrays. On the other hand, please do consider that looks is not everything and I don’t see this becoming much of a maintenance burden while I already found it very comfortable a couple of times during testing to be able to just copy and paste the URL which belonged to the JSON files that were causing trouble into Firefox and thus get very quick access to those documents. It certainly has advantages, which I hope you’ll agree on, so this is likely something that should be brought up again before the real and formal inclusion into the codebase (which might still be some time away from now).

-- 
http://www.fastmail.com - Does exactly what it says on the tin