maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Serg,

On Tue, Nov 02, 2021 at 02:19:05AM +0200, Sergei Krivonos wrote:
> Applied fixes by your tests patch 638930f85f… <https://github.com/MariaDB/server/commit/638930f85ff40bd39354da81e53e7278a4b28487>
> 

#1: Please check out the commit adding unit tests: 
http://lists.askmonty.org/pipermail/commits/2021-November/014773.html 
It turns out it is not that hard to add them. Any comments/ways to improve
this?

#2: In the above commit, note that two last unit tests show a scenario that is
not currently checked.

#3:  What is the point of functions Json_writer::on_add_str(), Json_writer::on_start_array, 
Json_writer::on_start_object() ? 
I would understand if all the checking code was isolated in these functions.
But it isn't. Please either remove the functions or explain the reasoning.


#4 See the input below about the commit comment. Please change (feel free to
force-push into the branch after that).


I think this is all the input, and the patch will be ready for pushing once all
of this is addressed.

> > Item #2: please improve the commit comment as was requested in my previous
> > e-mail:
> > 
> > On Thu, Oct 28, 2021 at 04:58:35PM +0300, Sergey Petrunia wrote:
> >> First, input on the commit comment. Please consider it as a request applying
> >> to ALL further commits to the MariaDB codebase.
> >> 
> >> 
> >>> MDEV-23766: implemented requested debug checks
> >> 
> >> Imagine somebody looking at this in a few years. Will they know what checks
> >> were requested? They might get a clue by looking at the Jira text, but the
> >> idea to make the commit comments concise and self-contained descriptions.
> >> 
> >> Something like this:
> >> 
> >> Line #1: one-line description of the patch:
> >> 
> >>>> MDEV-23766: Make attempts to write invalid JSON cause assertion failures
> >> 
> >> Subsequent lines:
> >> 
> >>>> Make JSON writing API (class Json_writer) check the produced JSON document
> >>>> is valid. The following checks are made:
> >>>> - JSON objects must contain named members
> >>>> - JSON arrays must contain unnamed members.
> >>>> - (TODO: add other restrictions we're enforcing).

BR
 Sergei
-- 
Sergei Petrunia, Software Developer
MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net