Re: [review][json] My review

From: Gavin Lambert (boost_at_[hidden])
Date: 2020-09-23 02:08:09

Mere moments ago, quoth I:
> On 23/09/2020 12:52, Vinnie Falco wrote:
>> It is the opposite of SRP violation. basic_parser is concerned only
>> with parsing and not keeping track of the full value. The
>> responsibility of tracking full values is moved to the caller. That
>> way, basic_parser does not impose its opinions on how the full value
>> should be stored. This was litigated in earlier posts if I recall
>> (Peter?).
> 
> Except it does, because it parses into int/uint/double, which is 
> inherently a storage decision.  You might have a valid argument if it 
> left that step to the handler to determine as well.  (i.e. only 
> providing on_number_part and on_number, just the text without any 
> attempt to parse into a numeric type.)

It's not just a storage *decision*; in order to implement this interface 
with the observed behaviour it also means that basic_parser must 
currently actually have *storage* for partial numbers updated across 
multiple fragments.  This is inconsistent with how it treats keys and 
strings.

Hence I reiterate: it would be more consistent to delete those methods 
and only have on_number_part + on_number that provide just the text. 
(Make it parser's responsibility to do the actual text-to-numeric-type 
parsing, ideally via a helper class that user code can call too.  Since 
it's the layer that's dictating the storage format via json::value.)

> The current form of basic_parser could still exist, if you like, but it 
> should be made an implementation detail and the complete-value 
> basic_parser actually exposed to consumers.

I still think that it would be better to instead provide an interface 
that precomposes the text fragments, but that is a separate argument 
from the prior one, that can be treated separately.

Perhaps you could provide both -- basic_parser as-is (but with the 
number parsing/storage removed), and another layer on top 
(custom_parser, perhaps) that is nearly identical but omits the _part 
methods to only provide precomposed keys/values.  This is the one that 
most consumers of a SAX-style interface would be expected to actually use.