-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: Add support for URL custom property value type #3879
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
base: master
Are you sure you want to change the base?
feat: Add support for URL custom property value type #3879
Conversation
|
It might make sense to add type for |
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: Lines 75 to 78 in 07ddcd9
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. |
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
|
@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. |
gmlewis
left a comment
There was a problem hiding this 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
Since
ValueTypeis 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