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

feat: removing private from typemapper #1674

Merged

Conversation

chollinger93
Copy link
Contributor

What

Resolves #1664

Makes TypeMappers no longer private case class fields, so we can use magnolia (or, potentially, other typeclass derivation libs) that rely on -Yretain-trees to derive default values.

Why

See #1664

I understand that this could be controversial, as it makes this a public API - but I would argue that all generated case class fields (including defaults) should be accessible by their owner.

Now, protobuf's generated code sits in an awkward spot between "owned by the person who generates and uses the scala code" and "owned by the person owning the proto files" (which, depending on your org shape, are likely to be 2 and more teams).

But, since it is scalapb that allows users to customize fields, effectively breaking the contract laid out by the .proto files, that makes the scala code owners the definitive owners of the generated files and, as such, the one who should have access to all fields.

Lastly, since proto fields are, by default, nullable/optional, having access to their default case class values is important.

Test

See https://github.com/chollinger93/scalapb-issue1664/tree/fix for a test repo + instructions

❯ diff before.scala.tmp after.scala.tmp
3,4d2
< //
< // Protofile syntax: PROTO3
178c176
<   private[nested] val _typemapper_action: _root_.scalapb.TypeMapper[test.nested.NestedExampleEvent.Action, String] = implicitly[_root_.scalapb.TypeMapper[test.nested.NestedExampleEvent.Action, String]]
---
>   val _typemapper_action: _root_.scalapb.TypeMapper[test.nested.NestedExampleEvent.Action, String] = implicitly[_root_.scalapb.TypeMapper[test.nested.NestedExampleEvent.Action, String]]

Before

 [error] -- [E173] Reference Error: /Users/christian/workspace/scalapb-issue1664/src/main/scala/Main.scala:10:58 
[error] 10 |case object ExampleThing extends Thing[NestedExampleEvent]:
[error]    |                                                          ^
[error]    |value _typemapper_action cannot be accessed as a member of test.nested.NestedExampleEvent.type from object ExampleThing.
[error]    |  private[nested] value _typemapper_action can only be accessed from package test.nested in package test.
[error]    |----------------------------------------------------------------------------
[error]    |Inline stack trace
[error]    |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
[error]    |This location contains code that was inlined from NestedProto.scala:13
[error] 13 |    action: String = test.nested.NestedExampleEvent._typemapper_action.toCustom(test.nested.NestedExampleEvent.Action.Undefined)
[error]    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]    |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
[error]    |This location contains code that was inlined from NestedProto.scala:13
[error]    |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
 

After

Hello, {"type":"record","name":"NestedExampleEvent","namespace":"test.nested","fields":[{"name":"id","type":"string","default":""},{"name":"action","type":"string","default":"Undefined"}]}
[success] Total time: 3 s, completed Apr 2, 2024, 9:28:53 AM

@thesamet
Copy link
Contributor

thesamet commented Apr 2, 2024

Thanks for putting together the PR. My preference would be to introduce this behavior as a file-level setting in scalapb.proto under ScalaPbOptions named optional bool public_constructor_parameters = nnn; . This can then be used as a package-level setting to apply for an entire package.

Doc for the new setting (in the proto and customizations.md) should indicate the risk of using it. Please see this doc on how to go about modifying scalapb.proto: https://github.com/scalapb/ScalaPB/blob/master/CONTRIBUTING.md#modifying-scalapbproto

Rational: most users would not need this feature: typemappers were introduced in Feb 2015 and this wasn't asked until now (Apr 2024), so assuming it's a fairly niche use case. Making the change to the public API would expose an internal implementation detail that could potentially break users, or just show unnecessarily in autocomplete, etc. Access to default values at runtime is available via Companion.defaultInstance, and if want to expose default values at compile time we may come up with a different mean down the road.

@chollinger93
Copy link
Contributor Author

chollinger93 commented Apr 2, 2024

Ty, makes sense! I'll put this together and update this PR once I get around to it, hopefully some time this week.

Rational: most users would not need this feature

Me, in this scenario:

image
https://xkcd.com/1172/

:-)

@chollinger93
Copy link
Contributor Author

@thesamet Updated this PR with the flag, an e2e test, and docs. Ran fmtAll + all other scripts.

@thesamet thesamet merged commit d93a990 into scalapb:master Apr 4, 2024
12 checks passed
@thesamet
Copy link
Contributor

thesamet commented Apr 4, 2024

Looks perfect! Merged.

thesamet pushed a commit that referenced this pull request Apr 7, 2024
* feat: removing private from typemapper

* feat: public_constructor_parameters flag
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

_typemapper are defined as package private, causing issues when deriving schemas
2 participants