Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix binary compatibility in FieldCodec factory methods #6380

Merged

Conversation

ObsidianMinor
Copy link
Contributor

Fix for #6379.

@jskeet
Copy link
Contributor

jskeet commented Jul 13, 2019

I'm confused about why we have these defaultValue parameters at all - they never seem to be used. My guess is that that's a separate bug, that the existing methods aren't calling the right constructor. (That should be confirmed with a breaking test before being fixed, and that should probably be in a different PR.)

I would make the overloads delegate to the methods accepting a default value though, unless there's a good reason not to.

@ObsidianMinor
Copy link
Contributor Author

that should probably be in a different PR

Right. I was going to do it in this one but decided to fix it in #5936 when the different default values would be put to use (with extensions being finalized then).

@TeBoring
Copy link
Contributor

Why previously there were no test to catch it?

@ObsidianMinor
Copy link
Contributor Author

There was no code to test against. There was no way to generate extension identifiers at the time (before #5936) so rather than add tests for code that couldn't actually be used (unless the consumer decided to implement the interfaces and use the API manually) I decided to wait until the last PR. Then the code could be fully tested properly when unittest.proto could actually be generated in the test project.

Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@jtattermusch
Copy link
Contributor

Fine to merge (and backport to v3.9.x branch)?

@anandolee anandolee merged commit 9857d63 into protocolbuffers:master Jul 19, 2019
jtattermusch pushed a commit to jtattermusch/protobuf that referenced this pull request Jul 22, 2019
…rs#6380)

* Fix binary compatibility in FieldCodec factory messages

* Make default value parameter for current factories required

* Route old methods through default value overloads
anandolee pushed a commit that referenced this pull request Jul 22, 2019
* Fix binary compatibility in FieldCodec factory messages

* Make default value parameter for current factories required

* Route old methods through default value overloads
anandolee added a commit that referenced this pull request Aug 9, 2019
* Add changelog for 3.9.x

* Revert "Make php message class final to avoid mocking (#6277)" (#6324)

This reverts commit 7f84a94.
This is just temporary. Eventually, we still want to roll forward this
change. Some users are complaining they need more time to clean up their
code.

* Update extract_includes.bat.in

File io_win32.h is not in directory google\protobuf\stubs under directory google\protobuf\io

* Set oneof case in array constructor (#6351)

Forgot to set it previously.

* Update protobuf version (#6366)

* Drop building wheel for python 3.4 (#6406)

https://github.com/matthew-brett/multibuild/pull/240

* Fix binary compatibility in FieldCodec factory methods (#6380) (#6424)

* Fix binary compatibility in FieldCodec factory messages

* Make default value parameter for current factories required

* Route old methods through default value overloads

* Remove ExtensionRegistry.Add(params) overload

* Rename ExtensionRegistry.Add(IEnumerable<Extension>) overload to AddRange

* Edit naming of parameters in Extension classes

* * Fix add API warnings to docs for extension APIs
* Rename internal ExtensionSet.GetValue to TryGetValue

* Disable javadoc error (#6371)

* Disable javadoc error

Actual fixes of the javadoc will be followed up

* Remove duplicated configuration

* Update javadoc plugin version

* Updated Bazel test script to use most recent Bazel version (#6413) (#6433)

I'm not exactly sure why, but this fixes the failing Bazel presubmit
test. Using the most recent version seems like a good idea anyway so
that we can make sure we're compatible with any new Bazel changes.

* [bazel] Add fixes for --incompatible_load_{cc,java,proto}_rules_from_bzl

* No need to update version in generated code (#6471)

generate_descriptor will handle that

* Update protobuf version (#6472)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants