Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Use both pactffi_set_comment and pactffi_add_text_comment #557

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

tienvx
Copy link
Contributor

@tienvx tienvx commented Apr 19, 2024

pactffi_set_comment does work, but there is a problem. pactffi_add_text_comment is a better replacement for it.

See these links for more information:

@tienvx tienvx requested a review from JP-Ellis April 19, 2024 03:55
@JP-Ellis
Copy link
Contributor

I think it might be best to extend the commenting functionality, instead of just replacing the current method. I did something similar in Python by adding two methods:

    def set_comment(self, key: str, value: Any | None) -> Self:  # noqa: ANN401
        """
        Set a comment for the interaction.

        This is used by V4 interactions to set a comment for the interaction. A
        comment consists of a key-value pair, where the key is a string and the
        value is anything that can be encoded as JSON.

        Args:
            key:
                Key for the comment.

            value:
                Value for the comment. This must be encodable using
                [`json.dumps(...)`][json.dumps], or an existing JSON string. The
                value of `None` will remove the comment with the given key.

        # Warning

        This function will overwrite any existing comment with the same key. In
        particular, the `text` key is used by `add_text_comment`.
        """
        if isinstance(value, str) or value is None:
            pact.v3.ffi.set_comment(self._handle, key, value)
        else:
            pact.v3.ffi.set_comment(self._handle, key, json.dumps(value))
        return self

    def add_text_comment(self, comment: str) -> Self:
        """
        Add a text comment for the interaction.

        This is used by V4 interactions to set arbitrary text comments for the
        interaction.

        Args:
            comment:
                Text of the comment.

        # Warning

        Internally, the comments are appended to an array under the `text`
        comment key. Care should be taken to ensure that conflicts are not
        introduced by
        [`set_comment`][pact.v3.interaction.Interaction.set_comment].
        """
        pact.v3.ffi.add_text_comment(self._handle, comment)
        return self

This allows end-users to add comments as per the original intentions behind the capability, while also allowing end-users to make use of the full capabilities of the Pact specification.

I would make sure to be really clear in the documentation of the possible way they can interact to hopefully avoid any unexpected side-effects.

@tienvx tienvx changed the title chore: Replace pactffi_set_comment by pactffi_add_text_comment chore: Use both pactffi_set_comment and pactffi_add_text_comment Apr 21, 2024
@tienvx
Copy link
Contributor Author

tienvx commented Apr 21, 2024

Added method pactffi_set_comment back

@tienvx tienvx merged commit 3d38623 into pact-foundation:master Apr 22, 2024
26 checks passed
@tienvx tienvx deleted the add-comment branch April 22, 2024 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants