1.10.0 has breaking changes

[jplaisted]

This PR https://github.com/github/twirp-ruby/pull/82, which was released in 1.10.0, introduced a breaking change and should have warranted a major version bump. Namely, the method signature of ClientResp#initialize was changed in a incompatible way (from positional arguments to keyword arguments).

I'm not sure what the path forward here is. Could revoke 1.10.0 (and re-release as 2.0.0), or add a warning and ask downstreams to pin to <= 1.9 ...

I appreciate it had been quite awhile since a release, and these things happen :) There may be other breaking changes, but that is one I noticed.

[jplaisted]

And to be clear, for this specific change, I doubt it will break production code (I hope no one is using ClientResp directly). But it may break a lot of test code; mocking out twirp clients to return a fudged ClientResp seems like it would be a common pattern.

[dbussink]

Can confirm that we use mocking as well where we construct a ClientResp with ClientResp.new which has broken the test suite. Also heads up @mikekavouras specifically as author of #82.

Specifically also interested if this is planned to stay as such and that we should update the test suite, or that this is considered too much of a breaking change and it's reverted. In that case it's easier to wait for a changed release instead of updating the test suite immediately.

[arthurnn]

Yeah, that is correct. We did the upgrade on GitHub, and had to fix some usages for ClientResp, where we were using the new method old signature.

The biggest issue in release a version 2 with this gem, is that the Go version follows along the versioning, and when releasing a go package with v2, we would have to update all the imports out there to include the /v2 suffix. (correct me if I am wrong here though)

An potential fix for folks that want to use the latest version of the gem, and can't do a full replace on the new signature would be:

class MockTwirpResp
  def self.new_client_resp(data, error)
    Twirp::ClientResp.new(data: , error: )
  end
end

Or something like that, and do a full replace of ClientResp.new with MockTwirpResp.new_client_resp.

I will definitely add a comment on the release notes for this case. Indeed, this should not break any production code, unless folks are mocking the response live, but, at least for GitHub we caught those errors on CI.

[dbussink]

The biggest issue in release a version 2 with this gem, is that the Go version follows along the versioning

I think this is the key here, since this makes it significantly harder to maintain semantic versioning. Since any change in any language means a new major for all languages. Maybe it's better to decouple this / let this go? And only do major bumps when the version actually needs to change for the specific language? Not sure what other things would depend on having cross language same versions?

[arthurnn]

And only do major bumps when the version actually needs to change for the specific language

@dbussink that is a very good point.

Not sure what other things would depend on having cross language same versions?

That is the only thing that have prevented me from doing it yet. I am not sure if there is any dependency on the cross lang versioning, and if there will be any complications.

[dpep]

I ran into this issue also 😞 a major version bump or warning in the CHANGELOG would have been helpful

Separately, after struggling a bit with rspec mocks + twirp, I wrote https://github.com/dpep/webmock-twirp to make testing easier. It stubs twirp requests / responses at the network level, thus side stepping this issue of mocking ClientResp and preserving all the type checking goodness of twirp/protobuf. Let me know what you think.

[jplaisted]

Since this has been open for 9 months and we're on a 1.11.0 now, seems like this won't be resolved. Hopefully someone added a notice in the update notes somewhere post hoc.