Skip to content

Conversation

@nberlette
Copy link
Contributor

Howdy! This is my first time contributing to this wonderful project, so please
bear with me if I make any mistakes or missed any conventions.

Now, onto the changes! I noticed the TODO comment in the create_formatter!
macro that mentioned de-duplicating the redundant overloads for user data.

My solution leverages Rust's optional parameters syntax $( ... )? — along with
a terribly-ugly little trick with Option — which eliminates the need for the
near-duplicate macro arm. As a bonus, I was also able to eliminate the extra
signatures for handling trailing commas. All in all this PR reduces the macro
down to just 3 signatures altogether!

The first two are the public-facing "overloads", and their usage and order
remains unchanged: the first pattern is "untyped" (i.e. does not add a custom
user payload to the generated function arguments), and the second accepts a
custom user type. The actual implementation is handled by the third (internal)
signature, which is easily discerned from the public ones by a special leading
@inner token.

I decided to add that token not only to help the compiler out, but also to
indicate to users that it is an internal API. Since the macro is recursive,
using a weird token like @inner (which should never accidentally appear in
a userland invocation) helps ensure it doesn't infinitely call itself.

Since there is no change in usage or syntax, I didn't add any tests for my
changes, but I'm more than happy to do so if needed!

If I missed anything or if there's any changes you'd like to see made to get
this thing fit to ship, please let me know and I'll get right on it.

Thanks!

Nick

Copilot AI review requested due to automatic review settings December 4, 2025 20:55
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR successfully refactors the create_formatter! macro to eliminate code duplication by consolidating multiple macro arms into a cleaner 3-arm structure. The refactoring uses Rust's optional macro parameter syntax $( ... )? and an @inner token pattern to handle both typed and untyped formatter creation through a single implementation.

Key changes:

  • Reduced macro from 4 arms to 3 arms by unifying the implementation logic
  • Introduced an @inner helper pattern to distinguish public API from internal implementation
  • Consolidated trailing comma handling into the main pattern matchers using $(,)?

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@kivikakk
Copy link
Owner

kivikakk commented Dec 5, 2025

Hey! Thank you so much for this wonderful contribution! I honestly didn't expect someone would read my "If you are reading this comment …" TODO comment and come along with a solution, and I'm so glad you did!

This looks excellent — +18 -85 is such a nice diffstat! — and yeah, I don't think any added tests are required, we cover this with examples. :)

Thank you, @nberlette! 🤍

@kivikakk kivikakk merged commit d6eeaa5 into kivikakk:main Dec 5, 2025
29 checks passed
@kivikakk
Copy link
Owner

kivikakk commented Dec 9, 2025

One thing I didn't realise at the time, but I am now in using it, is that this adds a Default requirement on the user data type which we didn't have before. This is despite it only being called when the type is (). Will think about ways to avoid this requirement.

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