Repository navigation
tighten TableForExtension readability - #5
Merged
Merged
Conversation
Unfinished — parked here so it stops riding along in the working tree during releases (a `gem build` from a dirty tree would publish code that is not in the tag). No behaviour change intended, only expression: * `footer_data` collapses the nil-guard and the call into one safe navigation, keeping the `defined?` memoization that caches a nil result. * `build_footer_cell_for` builds the class list declaratively with `compact_blank` + `presence` instead of a reject/join/ternary chain. * `column_key_for` returns early on the AA 4 `title_id` path and derives the AA 3 key with safe navigation. Green on both supported majors locally: 25 examples, 0 failures against activeadmin 3.5.1 and 4.0.0.beta23.
Fivell
changed the title
Fivell
marked this pull request as ready for review
October 20, 2026 07:33
There was a problem hiding this comment.
🟡 Changes recommended
A false footer-data callback now raises instead of returning and caching nil.
1 open issue
What changed in this PR
A readability refactor of TableForExtension, intended to preserve behavior across ActiveAdmin 3 and 4.
Changes:
- Simplifies footer-data callback invocation.
- Builds footer CSS classes declaratively.
- Streamlines column-key derivation.
| File | Description |
|---|---|
| lib/activeadmin_table_footer/table_for_extension.rb | Simplifies callback handling, footer classes, and column keys. |
Review effort: Balanced
The readability pass replaced the `unless @footer_data_proc` early return
with `&.`, which only skips nil. `table_for items, footer_data: false`
therefore raised NoMethodError instead of rendering no footer.
`false` reaches @footer_data_proc through a direct `table_for` call (the
dashboard case the array specs cover) when the callback is written as
`footer_data: flag && ->(c) { ... }`. The index wrapper already treats a
falsy callback as "no footer" (`footer_proc && page_presenter.block`), so
raising here was inconsistent as well as a regression.
Guard on truthiness again, keeping the one-expression shape and the
`defined?` memoization that caches a nil result.
Two unit examples cover it; the `false` one fails on the previous commit
with "undefined method 'call' for false". 27 examples, 0 failures on
activeadmin 3.5.1 and 4.0.0.beta23.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft — unfinished. Opened to get this out of the working tree: it was sitting there uncommitted during the 1.0.2 release, and
gem buildreads the working tree, so a release build would have published code that is not in the tag.Readability pass over
TableForExtension, no behaviour change intended:footer_data— collapse the nil-guard and the call into one safe navigation, keeping thedefined?memoization so anilresult still caches.build_footer_cell_for— build the class list declaratively withcompact_blank+presenceinstead of the reject/join/ternary chain.column_key_for— early return on the ActiveAdmin 4title_idpath, safe navigation for the ActiveAdmin 3 derived key.Green locally on both supported majors:
25 examples, 0 failuresagainst activeadmin 3.5.1 and 4.0.0.beta23.Before un-drafting
One intentional guard was dropped and deserves a second look:
column_key_forused to checkcol.title_id.respond_to?(:presence)before calling it, and now callscol.title_id.presenceas soon as the column responds totitle_id. That is equivalent fornilandString(both answerpresenceunder ActiveSupport), so it holds for stock ActiveAdmin — but it would break for atitle_idreturning some object withoutpresence. Worth deciding whether the original guard was defensive-by-accident or guarding a real case.