-
Notifications
You must be signed in to change notification settings - Fork 481
Split generated code into multiple files #1796
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
base: main
Are you sure you want to change the base?
Conversation
contents: filePrinter.main.content | ||
) | ||
try generatorOutputs.add( | ||
fileName: fileGenerator.outputFilename(".pb.hashable.swift"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the SwiftProtobuf sources, we have the convention that a file declaring an extension of some type gets named "Type+Protocol.swift"
I wonder if some variant of that convention would be more appropriate here? I think that would end up giving us ".pb+Hashable.swift" which is admittedly a little weird looking. But at a minimum, Hashable
and NameProviding
in these filenames should be correctly capitalized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my.pb+Hashable.swift
my.pb+NameProviding.swift
or
my+Hashable.pb.swift
my+NameProviding.pb.swift
?
Since some of these seems like they will only have protocol additions, do we run the risk of exposing problems like #1061 again? With #18 and #1240, one of the concerns can be how do things then present to developers. If the runtime still exposes the TextFormat and JSON methods of all messages, then someone not linking the right files will still build, and they would get a odd runtime failure. And it isn't just for the root message type, it would be an error from any of the transitive message types. So moving these to extensions means code can compile and fail at runtime which is sorta counter what I'd normally expect for a lot of Swift. It seems like it would be better if there was an way to ensure a developer couldn't try to call the methods if the support is going to be missing (atleast for the topmost message). |
What do you think about these things:
In theory it should not affect binary size, since this variable won't be used anywhere. |
Ultimately this is up to the Apple folks as they own the repo; my thoughts -
But as I mentioned, this is all up to the Apple folks in the end on what they'd like to do. ps - I'll also add that #1804 recently landed that shrankthe binary sizes some, and #1806 & #1807 also should have helped reduce some runtime costs. |
Code generation can significantly bloat the app size in large projects. Splitting generated code into multiple files allows teams to decide which features they want to include in their code.
Before
After