Skip to content

Conversation

@cosmicboots
Copy link
Contributor

This small PR includes the Mustache build results in the public API to allow developers to better annotate types and pass the build result around if needed.

The discussion for this originated on Discord, so I'll include the important bits below for future readers:

me: Is there a design decision behind why the MustacheBuildResult type isn't marked public?

renerocksai: At the time, I probably thought it wouldn't be necessary to make it public, as it is returned by a pub fn anyway, which makes it kind-of-public. This choice makes it impossible (I think) to declare variables of this type in user code and encourages just assigning the result of the build function.

Do you have a need for var x : zap.Mustache.MustacheBuildResult = .{} and assigning later or similar? Ah, maybe passing it around or storing it in your own struct: I would rather use its .str() result to hold on to.

Feel free to submit a PR to make it pub. I might do so myself; but then I'd strip the Mustache off the type names (type shaving 🤣) of the load args and the build result.

me: I've switched to passing around strings at this point (maybe that's better anyway...?), but I found it strange that I couldn't add annotations for a return type of a public function. Not saying I need it changed; I just found it slightly jarring from an API perspective.

@renerocksai
Copy link
Member

Thank you for bringing this up, fixing it, and documenting it so excellently! 👍

@renerocksai
Copy link
Member

I know it's a bit off-topic. But could you also rename the MustacheLoadArgs into LoadArgs? I am on vacation with limited computing capabilities.

@cosmicboots
Copy link
Contributor Author

@renerocksai I did some more type shaving in ee94128.

I hope you enjoy the rest of your vacation!

@renerocksai
Copy link
Member

Perfect! Thank you very much!!!!

@renerocksai renerocksai merged commit b77bada into zigzap:master Jul 15, 2024
@cosmicboots cosmicboots deleted the mustache-build branch July 23, 2024 21:33
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