Centralize CSV/style option parsing - #177
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Comma::Options.parse uses clone, which can raise on frozen input hashes, and the new nil→:default normalization changes observable behavior that should be explicitly resolved/aligned.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR centralizes parsing of the to_comma / Rails render csv: “style symbol vs options hash” argument into a single entry point (Comma::Options.parse) so both Comma::Generator and the Rails CSV renderer share consistent normalization (including write_headers string→boolean coercion).
Changes:
- Add
Comma::Options.parse(input)returning normalized{ style:, filename:, csv: }. - Update
Comma::Generator#initializeto delegate option parsing toComma::Options. - Remove Rails renderer’s bespoke
write_headersstring coercion and rely on the shared parsing path viaobj.to_comma.
File summaries
| File | Description |
|---|---|
lib/comma/options.rb |
Introduces centralized parsing/normalization of style/filename/CSV options (including write_headers coercion). |
lib/comma/generator.rb |
Switches generator initialization to consume parsed options from Comma::Options. |
lib/comma.rb |
Loads comma/options and removes duplicated renderer-side coercion logic. |
spec/comma/options_spec.rb |
Adds unit tests for Comma::Options.parse behavior and non-mutation. |
.rubocop_todo.yml |
Excludes the new file from Style/Documentation, matching existing lib/comma/* exclusions. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
64bb6b8 to
d55e82a
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Comma::Options.parse currently accepts non-Symbol non-Hash inputs (and treats false as “use default”), which can silently mask invalid arguments and should be tightened to the documented contract.
Review details
Suppressed comments (1)
lib/comma/options.rb:13
Comma::Options.parsecurrently treats any non-Hash input as a style value (includingfalse, strings, etc.), andfalsewill silently fall back to the default style because ofinput || .... Since this is intended to be the single entry point for parsing the style-vs-options argument, it should explicitly accept onlynil,Symbol, orHashand raise a clearArgumentErrorotherwise; while doing so, usedupinstead ofcloneso parsing doesn’t preservefrozen?state and explode when deleting keys.
def parse(input)
return { style: input || Comma::DEFAULT_OPTIONS[:style], filename: nil, csv: {} } unless input.is_a?(Hash)
csv = input.clone
style = csv.delete(:style) || Comma::DEFAULT_OPTIONS[:style]
filename = csv.delete(:filename)
csv[:write_headers] = csv[:write_headers] != 'false' if csv[:write_headers].is_a?(String)
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Generator and the Rails csv: renderer each parsed the style-vs-hash argument slightly differently, and the renderer carried a manual string-to-boolean hack for write_headers. Both now delegate to a single Comma::Options.parse, so there's one code path to keep correct and extend. Closes #163 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
d55e82a to
d103468
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The shared parser preserves existing behavior while reducing duplication, and the new unit specs plus existing controller specs cover the key normalization scenarios (including write_headers).
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
What
Adds
Comma::Options.parse(input)as the single entry point for parsing theto_comma/render csv:style-symbol-or-options-hash argument, and switchesComma::Generatorand the Railscsv:renderer to use it.Why
Comma::Generator#initializeand the Rails renderer inlib/comma.rbeach parsed the style-vs-hash argument slightly differently, and the renderer additionally carried a manual# XXX: Convert string to booleanhack forwrite_headers(needed because Rails params arrive as strings). The duplication meant a Comma-specific option added in one place could silently be missed in the other.Closes #163.
Changes
lib/comma/options.rb:Comma::Options.parseaccepts a bare style symbol, a hash ofstyle/filename/CSV options, or nil, and returns{ style:, filename:, csv: }. It also owns thewrite_headersstring→boolean coercion.lib/comma/generator.rb:Generator#initializenow delegates toComma::Options.parseinstead of inlining the Symbol/Hash branching.lib/comma.rb: the Rails:csvrenderer's manual write_headers string-coercion block is removed — it now shares the same normalization by routing throughGeneratorviaobj.to_comma.spec/comma/options_spec.rb: unit coverage for default/symbol/hash/style/filename/write_headers-coercion/non-mutation behavior..rubocop_todo.yml: addedlib/comma/options.rbto theStyle/Documentationexclusion, matching every other file underlib/comma/.No public API changes; existing
to_comma/render csv:call sites behave the same.Test plan
bundle exec rspec— 55 examples, 0 failures (46 pre-existing + 9 new)bundle exec rubocopclean on all changed filesspec/controllers/users_controller_spec.rb) — not runnable locally (native gem builds for the Rails appraisal gemfiles aren't set up in this environment); relies on CI, and I traced thewrite_headersstring/boolean/combo scenarios by hand against the new code to confirm no behavior change🤖 Generated with Claude Code