8000 include BuildResult in public Mustache API by cosmicboots · Pull Request #117 · zigzap/zap · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

include BuildResult in public Mustache API #117

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 15, 2024
Merged

include BuildResult in public Mustache API #117

merged 2 commits into from
Jul 15, 2024

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
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cosmicboots @renerocksai
0