fix: Interaction's comment should be string not array

[tienvx]

Currently I have to do this in pact-php to make the tests pass:

$this->interaction->setComments(['text' => json_encode([$value])]);

I think it should be this:

$this->interaction->setComments(['text' => $value]);

cc @JP-Ellis

[JP-Ellis]

I agree, there seemed to be a bit of a discrepancy when I implemented this. Should get @rholshausen to weigh in.

[rholshausen]

The format in the Pact file is an array so that multiple comments can be provided. For context see https://github.com/pact-foundation/pact-specification/issues/45

[JP-Ellis]

When I implemented the FFI, and taking into consideration that the comment field allows for any valid JSON value, I specifically have deserialised the input string.

For example:

FFI	Value
set_comment("a", "foo")	{"a": "foo"}
set_comment("a", '["foo"]')	{"a": ["foo"]}
set_comment("a", "true")	{"a": true}
set_comment("a", NULL)	{}
set_comment("a", "123")	{"a": 123}

Using set_comment with the same key will overwrite the key/value pair.

This is all tested in the test I implemented.

Perhaps this wasn't the intended way to specify comments? Though for the FFI, it ensures it is very flexible, allowing arbitrary content to be added.

Edit

Given the context of the issue you linked, would it be worth adding another FFI called add_text_comment which would append to the array under the "text" key? The implication would be that the content could be overwritten by set_comment. For example:

FFI	Value (consecutive)
set_comment("a", "foo")	{"a": "foo"}
add_text_comment("hello")	{"a": "foo", "text": ["hello"]}
add_text_comment("world")	{"a": "foo", "text": ["hello", "world"]}
set_comment("text", "123")	{"a": "foo", "text": 123}

The only thing here is that I don't particularly like having the redundancy between the two methods. As an end-user, I think each FFI should have a clear and distinct purpose, and set_comment and add_text_comment are too similar in my opinion.

[tienvx]

Not sure my approach is a good way:

Since:

• I assume only me and @JP-Ellis using this feature
• It is not intended way to add comment. We only allow end-user to modify comments -> text

May be remove set_comment(key, value) and add add_comment(value) to modify comments -> text only

[YOU54F]

hmmm so looking at the spec, its a map with a string for the key and a json value, which seems odd (it is that reason in the metaData section)

Looking at the original; request, should it be a string, or string array for our value, rather than json.

so Map[ string | List[string] ]

Field	Required?	Type	Description
comments	optional	Map[string, JSON]	Comments that are applied to the interaction. See comments section below

https://github.com/pact-foundation/pact-specification/tree/version-4?tab=readme-ov-file#comments

Key	Description
testname	Stores the name of the test that ran and generated the Pact file
text	List of free-form text comments to display

[YOU54F]

The only thing here is that I don't particularly like having the redundancy between the two methods. As an end-user, I think each FFI should have a clear and distinct purpose, and set_comment and add_text_comment are too similar in my opinion.

Agreed,

maybe as testName and comments are the examples in the spec, we could have them as dedicated methods along with one for arbritrary keys

setCommentTestName(string) setCommentText(string) setCommentTextArray(List[string]) setComment(key,string|List[string])

[JP-Ellis]

Given the Pact specification is quite explicit that the comments field is an optional field of type Map[string, JSON], then there should be a way of adding a boolean, string, array, or even an object within the comments.

Having said that, and as pointed out by Ron (as well as the spec itself), the text key within the comments does seem to be special in some way.

As a result, I think it might be best to leave the set_comment(key, value) as is in the FFI as it is a quite general function providing compatibility with the spec as it is written; and I can add a new add_text_comment(value) FFI method which will append text to the text key within the comments. This would ensure it works with the initial intent for comments as discussed in pact-foundation/pact-specification#45.

I would want to avoid having dedicated methods for each value type. Ultimately, this FFI should remain somewhat low level, and the libraries making use of the FFI can be responsible for the encoding of values into JSON strings.

[JP-Ellis]

Closing this as the underlying FFI addresses this issue.

EDIT: Just realised I can't close this 😅 @YOU54F or @rholshausen, could you do this?

[rholshausen]

Done!