adding arbitrary properties on schemafields

[barnjamin]

No description provided.

[serejja]

Shouldn't this be return nil, false?

[barnjamin]

yes, good catch

[serejja]

I'm a bit concerned about consistency of Prop() functions. All schema Prop() calls return string, bool whereas this PR introduces Prop() (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() returning string, 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 have string, bool in your case.

Thanks!

[barnjamin]

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.

[serejja]

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.

[barnjamin]

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?

[serejja]

Ok I've had a night to think about that :)

In my opinion we have two options:

• Breaking one. In this case I think it makes sense to tag/release the current master so that folks that wish to use the current approach could checkout to that tag/release. The master branch though would contain following changes:
  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 + SchemaField
• Non-breaking one. In this case we add one more map ArbitraryProperties 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!

[serejja]

@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]

https://github.com/stealthly/go-avro/releases/tag/v0.1.0.0

[crast]

@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.