breese / trial.protocol

Network wire protocols
11 stars 4 forks source link

Function parse_element #24

Closed vinipsmaker closed 6 years ago

vinipsmaker commented 6 years ago

Explained at https://github.com/breese/trial.protocol/issues/21#issuecomment-365286469

breese commented 6 years ago

I realized that this could be done by adding a json::parse(json::reader&) overload, which I have committed as d98eefd028f005c878daf59ab8abd87a57daf4ee

vinipsmaker commented 6 years ago

Just a few comments. This is a simple Python session:

Python 3.6.4 (default, Dec 23 2017, 19:07:07)
[GCC 7.2.1 20171128] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import json
>>> json.loads('{}')
{}
>>> json.loads('{}{')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.6/json/__init__.py", line 354, in loads
    return _default_decoder.decode(s)
  File "/usr/lib/python3.6/json/decoder.py", line 342, in decode
    raise JSONDecodeError("Extra data", s, end)
json.decoder.JSONDecodeError: Extra data: line 1 column 3 (char 2)
>>>

This behaviour will parse the document until the very end. I think this is the correct behaviour as it is safe by default (it'll avoid that you accidentally forward invalid JSON messages to 3rd parties).

So, parse and parse_element should behave differently. Therefore, I don't think they should be named the same to avoid confusion.

The implementation of parse could be as “forward task to parse_element and then ensure the current token after the call is json::token::code::end”. Several of these functions will be less than 5 lines and all will forward the real work to a single function, so it isn't a problem to ensure implementation quality (no code duplication and all tests will be against a single real useful function in the end).

breese commented 6 years ago

Good point about reporting an error if there is unparsed input left. I will add this.

Should we rename the overload that takes json::reader to partial_parse?

breese commented 6 years ago

Should we add a non-throwing version of the parse functions that returns errors as an error_code output parameter instead?

vinipsmaker commented 6 years ago

Should we rename the overload that takes json::reader to partial_parse?

I'd like to go for consistent naming so a library vocabulary emerges. You suggested skip_single in the other thread. I'd vote for parse_single here.

I think a json::parse(json::reader&) can still exist. It can be implemented in 4 lines with the parse_single function. And the presence of two functions will force the user to actually read the documentation to understand what is the difference.

dynamic::variable json::parse(json::reader &reader)
{
  auto ret = parse_single(reader);
  if (reader.symbol() != token::symbol::end)
    throw;
  return ret;
}

However, I also don't like the _single suffix. It gives the idea that the parse function is its opposite — parse_group — and I don't like that. Maybe _node is a better suffix. A node can be simple/leaf (number, string or boolean) or it can have children (arrays and objects).

vinipsmaker commented 6 years ago

Should we add a non-throwing version of the parse functions that returns errors as an error_code output parameter instead?

I should say to stay with the throwing version of the function only.