Copilot commented on code in PR #13609:
URL: https://github.com/apache/trafficserver/pull/13609#discussion_r3903072857
##########
include/mgmt/rpc/jsonrpc/json/YAMLCodec.h:
##########
@@ -28,9 +28,9 @@ namespace rpc::json_codecs
{
///
/// @note The overall design is to make this classes @c yamlcpp_json_decoder
and @c yamlcpp_json_encoder plugables into the Json Rpc
-/// encode/decode logic. yamlcpp does not give us all the behavior we need,
such as the way it handles the null values. Json needs
-/// to use literal null and yamlcpp uses ~. If this becomes a problem, then we
may need to change the codec implementation, we just
-/// follow the api and it should work with minimum changes.
+/// encode/decode logic. yamlcpp defaults to emitting null as ~, which is
valid yaml but not valid json, so every emitter here sets
+/// @c YAML::LowerNull to spell it literal null. Both spellings resolve back
to null when parsed as yaml, so accepting yaml input
+/// is unaffected.
Review Comment:
The new comment uses inconsistent capitalization/terminology for protocol
and format names. Consider updating to `YAML`/`JSON` (and possibly `yaml-cpp`
instead of `yamlcpp` if that’s the project’s standard) and a small grammar
tweak (e.g., 'spell it as literal null') to keep developer-facing documentation
crisp and consistent.
##########
doc/developer-guide/jsonrpc/jsonrpc-architecture.en.rst:
##########
@@ -74,6 +74,10 @@ Our JSONRPC protocol implementation uses lib yamlcpp for
parsing incoming and o
this allows the server to accept either JSON or YAML format messages which
then will be parsed by the protocol implementation. This seems handy
for user that want to feed |TS| with existing yaml configuration without the
need to translate yaml into json.
+Null values on the way out are emitted as literal ``null`` rather than yaml's
``~``, as ``~`` is not accepted by JSON parsers. Both
+spellings resolve to null when read as yaml, so this does not affect messages
that are consumed as yaml, nor the ability to send
+yaml to the server.
Review Comment:
For consistency with other documentation and common usage, consider
capitalizing `YAML` everywhere in this new paragraph (e.g., 'YAML's', 'read as
YAML', 'send YAML to the server'). This improves readability and reduces
ambiguity in docs.
##########
include/mgmt/rpc/jsonrpc/json/YAMLCodec.h:
##########
@@ -268,6 +268,7 @@ class yamlcpp_json_encoder
encode(const specs::RPCResponseInfo &resp)
{
YAML::Emitter json;
+ json.SetNullFormat(YAML::LowerNull);
json << YAML::DoubleQuoted << YAML::Flow;
encode(resp, json);
Review Comment:
The JSON emitter configuration (e.g., `SetNullFormat(LowerNull)` plus
`DoubleQuoted`/`Flow`) is now duplicated across multiple encoder entry points
in this PR. To reduce the risk of future emitters missing this required setup
(leading to regressions back to `~`), consider factoring this into a small
shared helper (e.g., `configure_json_emitter(YAML::Emitter&)`) and calling it
everywhere an emitter is created.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
