search

chrusty / protoc-gen-jsonschema

Protobuf to JSON-Schema compiler
Apache License 2.0
496 stars 101 forks source link

Put oneOf Types Update Behind Feature Flag #60

Closed grant closed 3 years ago

grant commented 3 years ago

If possible, could we put the oneOf feature (#57, #58) behind a feature flag? I believe the oneOf type is going to be a breaking change going forward, and isn't ideal for everyone. I'm running some language generators and the results aren't as great with this addition.

That might mean going back two commits and adding a forward commit with the feature behind a flag.

In general, it would be awesome if major features are behind a flag, such that any client can use the latest version of this tool and not see their generated schemas changing. Thank you!!

chrusty commented 3 years ago

Hi @grant.

This is a bit of a tricky discussion, and comes down to what makes a breaking change, and what is just adding functionality which arguably should be a default behaviour when interpreting proto files.

You've mentioned two recent additions here. I can see how https://github.com/chrusty/protoc-gen-jsonschema/pull/58 could modify behaviour in potentially annoying ways, but considering the spirit of "oneOf" it does kinda make sense for a default behaviour. Simply specifying the range of structrues that something could be one of isn't quite the same as ensuring that it can only be one of them, perhaps. I can see the point of a flag for this one.

The other commit though, that one won't break functionality... it just creates a more expressive schema.

So I have this dilemma. What is a breaking change? Should we just follow semver releases a bit more closely? There does need to be an allowance for bringing new things into the standard behaviour (because honestly, the flags are a bit annoying to work with).

Coming up to release 1.0.0 makes this an interesting discussion too (you can probably see by the large number of 0.9.x releases that I'm trying to buy some time here).

So how does this sound:

You can then pin to a specific tagged release, and update at your leisure (regenerating schemas, running your tests, and hopefully everything working alright).

grant commented 3 years ago

OK. #57 sounds fine. #58 behind a flag would help. I can't tell what affected the change on our side, those were just the PRs I see when diffing the last time we ran our tooling.

In general, we don't need to add a flag for everything – yes that would get annoying quickly.

The list above sounds good. I haven't followed the semver of this library much, but that plan sounds good.

chrusty commented 3 years ago

This is merged and tagged in a new release: https://github.com/chrusty/protoc-gen-jsonschema/releases/tag/0.9.10