3 ms·
Very glad to see the work that byroot is doing as the new ruby-json maintainer! Since I was mentioned by name in part 3, perhaps I can provide some interesting
by LukeShu 2y ago
Very glad to see the work that byroot is doing as the new ruby-json maintainer!
Since I was mentioned by name in part 3, perhaps I can provide some interesting commentary:
> All this code had recently been rewritten pretty much from scratch by Luke Shumaker ... While this code is very clean and generic, with a good separation of the multiple levels of abstractions, such as bytes and codepoints, that would make it very easy to extend the escaping logic, it isn’t taking advantage of many assumptions convert_UTF8_to_JSON could make to take shortcuts.
My rewritten version was already slightly faster than the original version, so I didn't feel the need to spend more time optimizing it, at least until the simple version got merged; which I had no idea when that'd be because of silence from the then-maintainer. Every optimization would be an opportunity for more pain when rebasing away merge-conflicts; which was already painful enough the 2 times I had to do it while waiting for a reply.
> One of these for instance is that there’s no point validating the UTF-8 encoding because Ruby did it for us and it’s impossible to end up inside convert_UTF8_to_JSON with invalid UTF-8.
I don't care to dig through the history to see exactly what changed when, but: At the time I wrote it, the unit tests told me that wasn't true; if I omitted the checks for invalid UTF-8, then the tests failed.
> Another is that there are only two multi-byte characters we care about, and both start with the same 0xE2 byte, so the decoding into codepoints is a bit superfluous. ... we can re-use Mame’s lookup table, but with a twist.
I noted in the original PR description that I thought a lookup table would be faster than my decoder. I didn't use a lookup table myself (1) to keep the initial version simple to make code-review simple to increase likelihood that it got merged, and (2) the old proprietary CVTUTF code used a lookup table, and because I was so familiar with the CVTUTF code, I didn't feel comfortable being the one to to re-add a lookup table. Glad to see that my suspicion was correct and that someone else did the work!
- JohnBooty 2y agoThanks so much for your work, and also thanks for some insight into choices you made. I'm not familiar with the internals of the JSON gem, but in general... yeah, it's funny right? PRs are almost never ideal. Always some compromise based on time available, code review considerations, etc. Everything you said makes a lot of sense!
- matheusmoreira 2y agoThanks for your work. I understand why you tried to keep it simple. Getting ignored or rejected by a maintainer is one of the least fun things I've ever experienced. Takes real skill to get something merged in, and not just technical skill.
- byroot 2y ago> At the time I wrote it, the unit tests told me that wasn't true Yes, it's something I changed before merging your patch. I didn't mean to say your patch wasn't good or anything It was very much appreciated.