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

Split text generation into different files #1240

Closed
wants to merge 3 commits into from

Conversation

thomasvl
Copy link
Collaborator

@thomasvl thomasvl commented Apr 7, 2022

This is a draft towards #18.

If we want to go in this directions, there's still a few things needed to be done:

  • decide if we want a generation option to not to control the behavior.
  • update the Package.swift and podspec to expose different targets for the base library support vs. the addition text support, then update the generation accordingly so the runtime parts aren't needed if not using the text support generated files.
  • double check if the Any support code can be further split. This likely is the any storage becoming something with an interface so when it comes in from a text for a different backing could be used vs. when it comes from binary.

@thomasvl
Copy link
Collaborator Author

If we split the library code so the text support is optional there also, which should be "SwiftProtobuf"? The full library with the text support or the main library with only the binary support? i.e. - do we add something like "SwiftProtobufCore" or "SwiftProtobufTextSupport"?

@tbkka
Copy link
Collaborator

tbkka commented Apr 12, 2022

I'm tempted to split it even finer, but I've not looked into how this actually would work in practice:

  • SwiftProtobufCore - Basic APIs only + binary coder/decoder
  • SwiftProtobufJSON - JSON coder/decoder
  • SwiftProtobufTextFormat - TextFormat coder/decoder
  • SwiftProtobuf - Umbrella that includes all of the above

"TextSupport" is likely to be confusing -- that looks like it only applies to TextFormat, not all text-based formats.

@thomasvl
Copy link
Collaborator Author

The naming support for enums (in its current form), has both TextFormat and JSON support. I believe trying to isolate that likely would make things larger in a bunch of cases when both are desired since it currently is optimized for overlapping cases to carry less data.

@thomasvl
Copy link
Collaborator Author

thomasvl commented Apr 12, 2022

I should add for completeness, should binary be optional? There might be some folks that only ever use JSON even though it isn't as good at evolving the schema with time.

edit: the generated code might not be different since we still need field numbers for the other formats; it would be more from a runtime library pov.

@thomasvl
Copy link
Collaborator Author

thomasvl commented Nov 4, 2022

@tbkka any more thoughts on the "TextSupport" vs. trying to split TextFormat and JSON, seems like that split would mean supporting both ends up with more bloat. Also, do we isolate binary so it is optional? One of the things I punted on originally was what to do with Any when things are optional, I guess all the conversion apis start failing if support for the needed types wasn't built in (just like if it wasn't registered)?

@thomasvl
Copy link
Collaborator Author

This still works for reference if anyone wants to look at picking up this work formally, but closing this PR since it's now somewhat out of date.

@thomasvl thomasvl closed this Sep 11, 2023
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.

2 participants