Closed barnjamin closed 8 years ago
I get that, I was thinking the same thing. Unfortunately I need an array for one of the properties and I'd like to avoid doing something like joining them as a string and splitting on some delimiter.
I can change the name of the function Prop to ArbitraryProp to draw a distinction. If thats the case, it may make sense to add that ArbitraryProp function to the other Schema structs and add it to the interface to allow the same functionality.
I like your approach. This way we could remove Prop in future releases (but not right now) because Prop and ArbitraryProp are nearly identical. I think it's ok to have them both for now not to break anyone and preserve consistency. Let's go for it.
All the Properties
maps in the Schema
structs are map[string]string
which doesn't support the ArbitraryProps
function. Do you want to add another map[string]interface{}
or change the current map and get rid of the type conversion in getProperties
func and push it to the Prop
func?
Ok I've had a night to think about that :)
In my opinion we have two options:
Properties map[string]string
becomes Properties map[string]interface{}
in all Schema implementations. This field is exported so there's a chance to break someone.
Prop(key string) (string, bool)
becomes Prop(key string) (interface{}, bool)
in all Schema implementations + SchemaFieldArbitraryProperties map[string]interface{}
besides the Properties map[string]string
and add ArbitraryProp(key string) (interface{}, bool)
to all Schema implementation + SchemaField.The second one looks a bit ugly and workaround-ish to me.
I actually think it makes sense to go with first approach before folks start using the Prop() (string, bool)
heavily. The less and earlier we break the better.
I'll talk to other collaborators today and let you know if they support this approach as well. Thanks!
@barnjamin ok we agreed on the first approach, I'll create a release right now and then we could accept that breaking change.
Thanks!
@serejja could you use semantic versioning for breaking changes? This way you also have a path down the road for using the gopkg.in service
I would also propose that if you're thinking of going down the road, break everything you plan on breaking soon, and then get a stable API going.
I'm a bit concerned about consistency of
Prop()
functions. All schemaProp()
calls returnstring, bool
whereas this PR introducesProp() (interface{}, bool)
.I'm not sure why we implemented the
string, bool
version the way it forces values to be strings but for consistency sake what do you think about having this new (*SchemaField).Prop() returningstring, bool
as well? I'm ok if you really need to be able to get anything and not just strings.I really don't want to make breaking changes so I'll rather leave it a bit inconsistent if you really need the
interface{}, bool
interface but please consider if it is possible to havestring, bool
in your case.Thanks!