Skip to content

Omit attributes when the value is an empty list or contains only nils #910

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

Merged

Conversation

z1lk
Copy link
Contributor

@z1lk z1lk commented Jun 9, 2025

Implements #909. I haven't benchmarked this but I'm hopeful about it. Because __attributes__ implicitly discards nil via the case value statement, we just have to make __nested_tokens__ resolve to nil, which is accomplished by returning early on an empty buffer. We can handle nested arrays and sets, as well as nested attributes, by checking for that nil return value in places where we call __nested_tokens__.

@joeldrapper
Copy link
Collaborator

This looks good. I can benchmark it. Will need to disable the attribute cache to get a useful result.

@z1lk z1lk force-pushed the omit-attributes-with-empty-token-lists branch from 502e2bd to bf827a8 Compare June 9, 2025 13:55
@z1lk
Copy link
Contributor Author

z1lk commented Jun 9, 2025

I'm curious how you'll disable attribute caching. It looks like there's no interface for disabling it currently, and I wonder if swapping the FIFO cache out for a no-op "null cache" would suffice for benchmarking these types of changes.

I'm also curious about the performance implications of __nested_tokens__ not taking the caller's existing buffer (like __nested_attributes__ does) and instead always allocating a new string to be returned. __nested_tokens__ could be updated to work like __nested_attributes__ but that would affect the changes here, because we would no longer care about the return value (nil or not). The method would just do its own thing with the buffer including adding the attribute name.


edit: I did some benchmarking with a null cache and a modified bench.rb and have come to the conclusions:

  • this PR doesn't degrade performance (but interested in your results)
  • __nested_tokens__ is fine as is. Any potential performance gain by passing a single string buffer around is more than offset by needing to gsub double quotes to HTML entity on individual tokens as they're pushed onto the buffer.

@joeldrapper
Copy link
Collaborator

Sorry I haven’t got back to this yet. Should be able to give Phlex some attention next week and will hopefully get this merged.

@z1lk z1lk force-pushed the omit-attributes-with-empty-token-lists branch from 7d0ddef to 561aa6e Compare July 17, 2025 19:57
@z1lk z1lk marked this pull request as ready for review July 17, 2025 20:36
@z1lk
Copy link
Contributor Author

z1lk commented Jul 17, 2025

Rebased and moved changes into the new attributes.rb.

Here's what I used to benchmark: z1lk@e4c130f Seeing similar results as main, with better average results on main over multiple runs.

Let me know if you need anything from me, and also feel free to take over the branch if you'd like.

(Not dying to get this merged, just checked in and saw this could be rebased.)

@joeldrapper joeldrapper merged commit 4c4abaa into yippee-fun:main Jul 21, 2025
12 of 13 checks passed
@joeldrapper
Copy link
Collaborator

Thank you.

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