[vlc-devel] [vlc-commits] JSON representation and Javascript unescaping

[Thomas Guillem]

On Mon, Sep 28, 2020, at 15:26, Rémi Denis-Courmont wrote:
> Hi,
> 
> No. Code review would have changed exactly nothing here.
> 
> I was never going to touch or use the awful existing JSON code. It's not just buggy, it's barely legible. And changing existing users is totally out of scope of the YoutubeDL patchset, in every way. What Alexandre said, again.

Avoiding code duplication is important. It's not a blocker but it's disputable. I don't see why developers should not touch at some part of the code.

> And it's pretty rich of somebody who claims to be a nice guy to single me out not once but twice. 

I'm not a nice guy, I'm just dreaming of a perfect world where everyone goes through a proper review process.

> Congratulations because I'm even more disgusted and even less likely to go through code review onward, than I already was from other people's last week's obstructionism.

I don't see obstructionism, lot of people were interested, some of us didn't fully understand the implication of such patch (some us didn't even know that ytdl handled more than 100 sites and not only one). I was personally afraid of the performance consequence (but I was still very enthusiastic by your patch set).

In the end, your patches were merged in around one week, it looks like a decent review process to me.

> 
> Le 28 septembre 2020 16:04:00 GMT+03:00, Thomas Guillem <thomas at gllm.fr> a écrit :
>> That is why code review is important.
>> 
>> We would have said to either fix the "dubious" JSON parser, or remove it (and fix the old JSON parser user to use the new one). We should avoid code duplication at maximum, remember the adaptive HTTP discussions...
>> 
>> And indeed, getting code merged can be a very long and fastidious process. For small patch set, it can take 1 week, sometimes it takes months on bigger ones.
>> 
>> On Mon, Sep 28, 2020, at 14:26, Rémi Denis-Courmont wrote:
>>> https://mailman.videolan.org/pipermail/vlc-devel/2020-September/137937.html
>>> 
>>> Le 28 septembre 2020 15:17:49 GMT+03:00, Francois Cartegnie <fcvlcdev at free.fr> a écrit :
>>>> Le 28/09/2020 à 13:28, Rémi Denis-Courmont a écrit :
>>>>> Hi,
>>>>> 
>>>>> That was already mentioned in one of the review thread. Also that that code is horrible, with its at best dubious pointer casts, most likely aliasing bugs.
>>>> 
>>>> ????
>>>> I see no thread about json except the one from today about the commits.
>>>> 
>>>> The last subject mentioning json was on 27/08 about lua/http.
>>>> 
>>>>> And looking at it again, it seems to decode strings incorrectly to CESU-8 instead of UTF-8.
>>>> 
>>> 
>>> -- 
>>> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté. 
>>> _______________________________________________
>>> vlc-devel mailing list
>>> To unsubscribe or modify your subscription options:
>>> https://mailman.videolan.org/listinfo/vlc-devel
>> 
> 
> -- 
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté. 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200928/3df21c0f/attachment.html>