google
The `DefaultValue` of a `CustomProperty` is incorrect type
Since GitHub added support for multi-select it's been possible for the DefaultValue of a CustomProperty to be either a string or an array of strings; see schema below.
"default_value": {
"oneOf": [
{
"type": "string"
},
{
"type": "array",
"items": {
"type": "string"
}
}
],
"description": "Default value of the property",
"type": [
"null",
"string",
"array"
]
},
Based on the existing implementation of CustomPropertyValue where Value has the same type we should modify DefaultValue to any.
This would be a great PR for any new contributor to this repo or a new Go developer. All contributions are greatly appreciated!
Feel free to volunteer for any issue and the issue can be assigned to you so that others don't attempt to duplicate the work.
Also feel free to perform a code review on any PR that has the label "NeedsReview" - we appreciate those contributions too!
We have a REVIEWERS file that you could add your username to if you are willing to be contacted when reviews are needed.
Please check out our CONTRIBUTING.md guide to get started. (In particular, please remember to go generate ./... and don't use force-push to your PRs.)
Thank you!
@zyfy29 - please make a note in issues when you decide to work on them so that others will not attempt to work on it at the same time as you. Thank you!
I don't think switching the default value to any is the right API design here. The GitHub type system isn't well aligned to Go, but there are alternatives to falling back to any. My suggestion would be to add a DefaultValues: []string struct field and use conditional marshalling (as I used for rulests) to set the correct field depending on the property type.
It's worth noting that this pattern is a non-breaking API change.
My only concern with attempting to strongly-type the DefaultValue field here is that GitHub may expand the set of what is allowable over time.
You originally wrote in the description above:
Based on the existing implementation of
CustomPropertyValuewhereValuehas the same type we should modifyDefaultValuetoany.
and I'm now confused if I'm misunderstanding your proposal or if you changed your mind. Could you please clarify, @stevehipwell ?
@gmlewis my initial suggestion was to have something like below where we just added a new field to handle the additonal case.
type CustomProperty struct {
...
// Default value of the property if the type is string or single_select.
DefaultValue *string
// Default values of the property if the type is multi_select.
DefaultValues []string
}
But on further reflection I think a new type would make this cleaner and cover the potential for new patterns. This new type would be custom marshalled and have access functions for the specific types. It could also be used for the actual value instead of the current any which would make the whole API more intuitive to work with.
type CustomProperty struct {
...
// Default value of the property.
DefaultValue *PropertyValue
}
type PropertyValue struct {
valueType string
value any
}
func (pv PropertyValue) StringValue() (string, bool) {
switch pv.valueType {
case "string", "single_select":
return pv.value.(string), true
default:
return "", false
}
}
Unfortunately the current naming makes naming the new type non-obvious, PropertyValue is just a placeholder for the purpose of the discussion.
Having the two fields valueType string and value any (with type casting) seems a little odd to me when you could simply add a single nullable-field for every possible value type, then check which one isn't nil with a switch to return it.
For example:
type PropertyValue struct {
stringValue *string
stringSliceValue *[]string
}
func (pv PropertyValue) StringValue() (string, bool) {
switch {
case pv.stringValue != nil:
return *pv.stringValue, true
default:
return "", false
}
}
func (pv PropertyValue) StringSliceValue() ([]string, bool) {
switch {
case pv.stringSliceValue != nil:
return *pv.stringSliceValue, true
default:
return "", false
}
}
@gmlewis that pattern would allow multiple values to be set and then the result would be determined by the order of the checks. I guess we could make DefaultValue into any and add functions to CustomProperty to cast based on property type and not need custom marshaling.
type CustomProperty struct {
...
// Default value of the property.
DefaultValue any
}
func (cp CustomProperty) StringDefaultValue() (string, bool) {
switch cp.DefaultValue {
case "string", "single_select":
s, ok := cp.DefaultValue.(string)
return s, ok
default:
return "", false
}
}
@gmlewis that pattern would allow multiple values to be set and then the result would be determined by the order of the checks. I guess we could make
DefaultValueintoanyand add functions toCustomPropertyto cast based on property type and not need custom marshaling.type CustomProperty struct { ... // Default value of the property. DefaultValue any }
func (cp CustomProperty) StringDefaultValue() (string, bool) { switch cp.DefaultValue { case "string", "single_select": s, ok := cp.DefaultValue.(string) return s, ok default: return "", false } }
No, that won't work as written.
No, that won't work as written.
@gmlewis sorry, won't work at a high level or the specific example code?
Personally I like the idea of a type to allow the logic to be re-used, but it adds complexity to the whole system. Your implementation pattern above would be the simplest to keep consistent and assuming we require a set function for package users we would be able to handle the internal state.
type PropertyValue struct {
stringValue *string
stringSliceValue *[]string
}
func (pv *PropertyValue) StringValue() (string, bool) {
switch {
case pv.stringValue != nil:
return *pv.stringValue, true
default:
return "", false
}
}
func (pv *PropertyValue) SetStringValue(s string) {
pv.stringValue = Ptr(s)
pv.stringSliceValue = nil
}
@gmlewis sorry, won't work at a high level or the specific example code?
Sorry, @stevehipwell - I was on my phone when I made the short comment.
The following code will return one of 3 values (if it compiles):
"string", true"single_select", true"",false
Nothing else. That's all I was saying. I'll have to look at your most recent comment later when I get a chance.
type CustomProperty struct {
...
// Default value of the property.
DefaultValue any
}
func (cp CustomProperty) StringDefaultValue() (string, bool) {
switch cp.DefaultValue {
case "string", "single_select":
s, ok := cp.DefaultValue.(string)
return s, ok
default:
return "", false
}
}
@gmlewis sorry, I was just free typing. What I meant to type.
type CustomProperty struct {
...
// Default value of the property.
DefaultValue any
}
func (cp CustomProperty) StringDefaultValue() (string, bool) {
switch cp.ValueType{
case "string", "single_select":
s, ok := cp.DefaultValue.(string)
return s, ok
default:
return "", false
}
}
OK, if you prefer that route, then I would definitely make the ValueType its own type (with its underlying type still being string) with const enumerated values, but I'm fine with that otherwise.