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

Add bufStrftime() convenience function #10

Closed
wants to merge 1 commit into from

Conversation

reykjalin
Copy link

Description

Being new to Zig I found the anywriter interface confusing, and it took me a while to figure out.
After figuring it out I found the boilerplate with setting up a writer distracting, so I figured something that can write straight to a buffer, similar to std.fmt.bufPrint would be nice to have?

The changes didn't take long so I wouldn't mind doing the same thing for gofmt() if you like the idea of this.

Also happy to make any changes if you think this has some merit but needs work ☺️

Changes:

  • Add bufStrftime() function as a wrapper around strftime().
  • Same unit tests run for strftime() and bufStrftime().
  • Added an example to the readme.

// Format using strftime straight to buffer.
var buf: [128]u8 = undefined;
const formatted_date_string = try dt.bufStrftime(buf, "%Y-%m-%d %H:%M:%S %Z");

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced...I don't think expanding the API of zeit is worth saving two lines of code at the callsite:

    var buf: 128[u8] = undefined;
    var fbs = std.io.fixedBufferStream(&buf);
    try dt.strftime(fbs.writer().any(), "%Y-%m-%d %H:%M:%S %Z");
    const formatted_date_string = fbs.getWritten();

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's fair! I can definitely respect that :)

As an alternative, maybe there should be a fixedBufferStream example in the readme? It would've made my life easier as someone new to zig 🤔

Knowing more about what anywriter means now I can absolutely see how that would be enough for anyone familiar with zig, so adding a bigger example to the readme would probably only benefit people new to zig.

The example I'm thinking of is exactly what you've outlined in this thread, so it wouldn't be a big addition.

@rockorager
Copy link
Owner

Thanks for the PR! Left a couple comments, overall I'm not thinking I'll accept this one - I want to keep the API pretty small and I'm not convinced this one is worth expanding it

@reykjalin
Copy link
Author

overall I'm not thinking I'll accept this one - I want to keep the API pretty small and I'm not convinced this one is worth expanding it

Totally respect that, I'll close it. Thanks for considering it!

@reykjalin reykjalin closed this Oct 16, 2024
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