Copilot commented on code in PR #13609:
URL: https://github.com/apache/trafficserver/pull/13609#discussion_r3905773773
##########
include/tsutil/YamlCfg.h:
##########
@@ -39,6 +39,22 @@ namespace Yaml
constexpr std::string_view YAML_BOOL_TAG_URI{"tag:yaml.org,2002:bool"};
constexpr std::string_view YAML_NULL_TAG_URI{"tag:yaml.org,2002:null"};
+ // Put an emitter into JSON output mode.
+ //
+ // yaml-cpp has no JSON mode. The nearest equivalent is flow style with
every scalar double quoted, which is valid
+ // JSON for every node type except null: yaml-cpp writes ~, and no JSON
parser accepts that. LowerNull writes the
+ // literal null. YAML reads ~ and null as the same value, so output stays
readable as YAML either way.
+ //
+ // Every emitter whose output reaches a JSON consumer must go through here.
Setting two of the three manipulators
+ // gives output that looks like JSON and parses correctly until some node is
null.
Review Comment:
The comment overstates correctness as “valid JSON for every node type except
null”. yaml-cpp can emit YAML-specific constructs (e.g., tags/anchors/aliases)
or styles depending on node state, which are not JSON. Recommend rewording to
clarify this produces JSON-compatible output for the subset of nodes your
codecs emit (and that `LowerNull` specifically addresses the `~` vs `null`
incompatibility).
##########
src/traffic_ctl/CtrlPrinters.cc:
##########
@@ -102,7 +102,7 @@ void
BasePrinter::write_output_json(YAML::Node const &node) const
{
YAML::Emitter out;
- out << YAML::DoubleQuoted << YAML::Flow;
+ ts::Yaml::configure_json_emitter(out);
Review Comment:
`ts::Yaml::configure_json_emitter` is now used here; please add a direct
include for its declaration (e.g., `#include \"tsutil/YamlCfg.h\"`) in this
`.cc` to avoid relying on transitive includes and to prevent fragile build
breaks when include graphs change.
##########
include/mgmt/rpc/jsonrpc/json/YAMLCodec.h:
##########
@@ -21,16 +21,18 @@
#pragma once
#include <yaml-cpp/yaml.h>
+
+#include "tsutil/YamlCfg.h"
#include "mgmt/rpc/jsonrpc/error/RPCError.h"
#include "mgmt/rpc/jsonrpc/Defs.h"
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:
For consistency/clarity in user-facing comments, consider using the
canonical library name and capitalization (e.g., “yaml-cpp”, “YAML”, “JSON”)
and formatting `~` as code. This reduces ambiguity in docs and matches the
terminology used elsewhere in the repo.
--
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]
