Skip to content

Conversation

@IrvingMg
Copy link

Since ValueType is just a string field with no validation, I only updated the documentation comment and added a test. Let me know if I'm missing something.

Fixes #3878

@stevehipwell
Copy link
Contributor

It might make sense to add type for ValueType?

@IrvingMg
Copy link
Author

It might make sense to add type for ValueType?

I think it would make sense. However, as I see it, the pattern in the library is to use similar enum-like fields as plain strings with documentation comments, such as:

SquashMergeCommitTitle *string `json:"squash_merge_commit_title,omitempty"` // Can be one of: "PR_TITLE", "COMMIT_OR_PR_TITLE"
SquashMergeCommitMessage *string `json:"squash_merge_commit_message,omitempty"` // Can be one of: "PR_BODY", "COMMIT_MESSAGES", "BLANK"
MergeCommitTitle *string `json:"merge_commit_title,omitempty"` // Can be one of: "PR_TITLE", "MERGE_MESSAGE"
MergeCommitMessage *string `json:"merge_commit_message,omitempty"` // Can be one of: "PR_BODY", "PR_TITLE", "BLANK"

But if you’re okay with adding types, I could add constants like:

const (
      PropertyValueTypeString      = "string"
      PropertyValueTypeSingleSelect = "single_select"
      PropertyValueTypeMultiSelect  = "multi_select"
      PropertyValueTypeTrueFalse    = "true_false"
      PropertyValueTypeURL          = "url"
  )

And we can decide later if we want to add similar enums for other fields across the library.

@gmlewis gmlewis changed the title Add support for URL custom property value type feat: Add support for URL custom property value type Dec 13, 2025
@codecov
Copy link

codecov bot commented Dec 13, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.48%. Comparing base (07ddcd9) to head (1356164).
⚠️ Report is 12 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3879   +/-   ##
=======================================
  Coverage   92.48%   92.48%           
=======================================
  Files         200      200           
  Lines       14564    14564           
=======================================
  Hits        13469    13469           
  Misses        895      895           
  Partials      200      200           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@stevehipwell
Copy link
Contributor

@IrvingMg this pattern is used elsewhere here.

@IrvingMg
Copy link
Author

@IrvingMg this pattern is used elsewhere here.

I’ve added the constants. I considered using a type alias, but I believe that would be a breaking change, and I wasn’t sure we wanted to introduce one.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

LGTM.
Awaiting second LGTM+Approval from any other contributor before merging.

cc: @stevehipwell - @alexandear - @zyfy29

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Dec 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NeedsReview PR is awaiting a review before merging.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for URL custom properties

3 participants