Skip to content

Conversation

alank-ps
Copy link
Contributor

@alank-ps alank-ps commented Jun 6, 2025

…content types also JSON

#1161

@alank-ps alank-ps requested a review from a team as a code owner June 6, 2025 08:21
@alank-ps alank-ps requested a review from embano1 June 10, 2025 23:27
alank-ps added 2 commits June 11, 2025 11:18
…content types also JSON

Signed-off-by: Alan Kan <alan.kan@pepperstone.com>
Signed-off-by: Alan Kan <alan.kan@pepperstone.com>
@alank-ps
Copy link
Contributor Author

Hi @embano1 , do you mind having another look?

@@ -14,6 +14,20 @@ const (
ApplicationCloudEventsBatchJSON = "application/cloudevents-batch+json"
)

type ContentType string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, can we 1/ not export this and 2/ simplify this into just a simple function? See below.

type ContentType string

// IsJSON returns true if the content type is a JSON type.
func (c ContentType) IsJSON() bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of introducing an exported type, how about:

Suggested change
func (c ContentType) IsJSON() bool {
func isJSON(contentType string) bool { ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unit test cannot access it then :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can ditch the unit test if preferred

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@embano1 updated as suggested, thanks 🙇

Signed-off-by: Alan Kan <alan.kan@pepperstone.com>
@alank-ps alank-ps requested a review from embano1 June 12, 2025 07:55
@embano1 embano1 enabled auto-merge June 12, 2025 08:35
Copy link
Member

@embano1 embano1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx

@embano1 embano1 merged commit 65b45e4 into cloudevents:main Jun 12, 2025
9 checks passed
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