Add safeyaml.YAMLUnmarshalerToJSONBytes

[bmoylan]

Reverse of #139

---

This change is 

[nmiyake]

Content looks good! Minor feedback on documentation and test cases.

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @bmoylan)

---

safeyaml/yamltojson.go, line 13 at r1 (raw file):

)

// YAMLUnmarshalerToJSONBytes decodes YAML bytes into an interface{} where all objects and maps are decoded

The first sentence/top-level description appears to be either incorrect or incomplete -- the description says that the function decodes YAML bytes into an interface{}, but the return value for the function is a byte slice?

---

safeyaml/yamltojson.go, line 14 at r1 (raw file):

// YAMLUnmarshalerToJSONBytes decodes YAML bytes into an interface{} where all objects and maps are decoded
// as a map[string]interface{} for json compatibility. This can be used in an UnmarshalYAML

json -> JSON

---

safeyaml/yamltojson.go, line 16 at r1 (raw file):

// as a map[string]interface{} for json compatibility. This can be used in an UnmarshalYAML
// implementation where the type implements custom UnmarshalYAML behavior and wants it to apply
// to yaml as well. For example:

YAML

---

safeyaml/yamltojson.go, line 32 at r1 (raw file):

	}

	// YAML objects are not completely compatible with JSON objects (e.g. you

Nit: rephrase to remove the "you" (something like "(e.g. YAML may have non-string keys)").

---

safeyaml/yamltojson.go, line 46 at r1 (raw file):

// YAMLtoJSONBytes converts YAML content to JSON.
// YAML objects are not completely compatible with JSON objects (e.g. you

The sentences after the first one are copy-pasted from the impl and seem lower-level than necessary here. I think something like "Returns an error if the provided YAML bytes cannot be represented as JSON (for example, if it contains non-string keys)."

Doesn't have to be that exact wording, but would basically focus on what the conditions are in which an error is returned.

---

safeyaml/yamltojson_test.go, line 18 at r1 (raw file):

		YAML string
		JSON string
		Err  string

Can we add at least 1 case where an error is returned based on the content (the case described in code with a non-string key, for example)?

[bmoylan]

Reviewable status: 2 of 4 files reviewed, 5 unresolved discussions (waiting on @nmiyake)

---

safeyaml/yamltojson.go, line 13 at r1 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

The first sentence/top-level description appears to be either incorrect or incomplete -- the description says that the function decodes YAML bytes into an interface{}, but the return value for the function is a byte slice?

Done.

---

safeyaml/yamltojson.go, line 14 at r1 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

json -> JSON

Done.

---

safeyaml/yamltojson.go, line 16 at r1 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

YAML

Done.

---

safeyaml/yamltojson.go, line 32 at r1 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

Nit: rephrase to remove the "you" (something like "(e.g. YAML may have non-string keys)").

Done.

---

safeyaml/yamltojson.go, line 46 at r1 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

The sentences after the first one are copy-pasted from the impl and seem lower-level than necessary here. I think something like "Returns an error if the provided YAML bytes cannot be represented as JSON (for example, if it contains non-string keys)."

Doesn't have to be that exact wording, but would basically focus on what the conditions are in which an error is returned.

Done.

---

safeyaml/yamltojson_test.go, line 18 at r1 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

Can we add at least 1 case where an error is returned based on the content (the case described in code with a non-string key, for example)?

Done.

[nmiyake]

CI is failing (looks like test code needs to be formatted)

[bmoylan]

thanks, done

[nmiyake]

Thanks!

Did another read-through and had API questions about the piece that performs the json.Marshal (if the scope of our use of this API is limited then having ability to customize encoder may not be an issue, but flagging since this is something I've encountered when writing/using APIs like this in the past).

Also some other minor documentation feedback.

Reviewed 2 of 2 files at r2.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @bmoylan and @nmiyake)

---

safeyaml/yamltojson.go, line 13 at r2 (raw file):

)

// YAMLUnmarshalerToJSONBytes decodes YAML bytes (in the form of an unmarshal lambda provided by go-yaml)

Would suggest a construction where the first sentence fully describes the behavior and then the rest of the comment expounds on it.

So, in this case, something like:

YAMLUnmarshalerToJSONBytes decodes YAML bytes using the provided unmarshal function, converts the object to a JSON-compatible representation and returns the result of marshalling the converted object as JSON.

Further implementation details etc. can be added afterwards, but think it's important for the first overview sentence to describe the full workflow/result.

nit that the input is not necessarily a lambda (it can be a named function).

---

safeyaml/yamltojson.go, line 26 at r2 (raw file):

//		return json.Unmarshal(jsonBytes, o)
//	}
func YAMLUnmarshalerToJSONBytes(unmarshal func(interface{}) error) ([]byte, error) {

In thinking about the feedback for the documentation, I think that UnmarshalerToJSONBytes might be a more accurate name here -- I don't think that any behavior

---

safeyaml/yamltojson.go, line 43 at r2 (raw file):

	// Convert this object to JSON and return the data.
	return json.Marshal(jsonObj)

Just thinking about this now -- is there ever a scenario where we would want to control the encoder settings for the output JSON? If that's the case, we may want an API that either returns the object (so that caller can do whatever they want with it) or a version that accepts an encoder (so that caller can control exactly how encoding is performed).

For example, the default json.Marshal has SetEscapeHTML (https://golang.org/pkg/encoding/json/#Encoder.SetEscapeHTML), which I know has bitten us in some scenarios in the past

[bmoylan]

Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @nmiyake)

---

safeyaml/yamltojson.go, line 13 at r2 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

Would suggest a construction where the first sentence fully describes the behavior and then the rest of the comment expounds on it.

So, in this case, something like:

YAMLUnmarshalerToJSONBytes decodes YAML bytes using the provided unmarshal function, converts the object to a JSON-compatible representation and returns the result of marshalling the converted object as JSON.

Further implementation details etc. can be added afterwards, but think it's important for the first overview sentence to describe the full workflow/result.

nit that the input is not necessarily a lambda (it can be a named function).

Done.

---

safeyaml/yamltojson.go, line 26 at r2 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

In thinking about the feedback for the documentation, I think that UnmarshalerToJSONBytes might be a more accurate name here -- I don't think that any behavior

Done.

---

safeyaml/yamltojson.go, line 43 at r2 (raw file):

Previously, nmiyake (Nick Miyake) wrote…

Just thinking about this now -- is there ever a scenario where we would want to control the encoder settings for the output JSON? If that's the case, we may want an API that either returns the object (so that caller can do whatever they want with it) or a version that accepts an encoder (so that caller can control exactly how encoding is performed).

For example, the default json.Marshal has SetEscapeHTML (https://golang.org/pkg/encoding/json/#Encoder.SetEscapeHTML), which I know has bitten us in some scenarios in the past

Done.

[nmiyake]

Reviewed 1 of 1 files at r3, 1 of 1 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved