From d103468484bcf82b8a86dd2fb6fb6d2fff9a649e Mon Sep 17 00:00:00 2001 From: Eito Katagiri Date: Sat, 5 Sep 2026 18:12:54 +0900 Subject: [PATCH] Centralize CSV/style option parsing into Comma::Options 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 --- .rubocop_todo.yml | 1 + lib/comma.rb | 10 +----- lib/comma/generator.rb | 12 +++----- lib/comma/options.rb | 18 +++++++++++ spec/comma/options_spec.rb | 63 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 87 insertions(+), 17 deletions(-) create mode 100644 lib/comma/options.rb create mode 100644 spec/comma/options_spec.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 8a6bf3e8..a024c1c8 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -122,6 +122,7 @@ Style/Documentation: - 'lib/comma/header_extractor.rb' - 'lib/comma/mongoid.rb' - 'lib/comma/object.rb' + - 'lib/comma/options.rb' - 'lib/comma/relation.rb' # Offense count: 2 diff --git a/lib/comma.rb b/lib/comma.rb index 577c8318..9f398196 100644 --- a/lib/comma.rb +++ b/lib/comma.rb @@ -23,6 +23,7 @@ module Comma require 'comma/data_mapper_collection' if defined? DataMapper +require 'comma/options' require 'comma/generator' require 'comma/array' require 'comma/object' @@ -42,15 +43,6 @@ module Comma # Capture any CSV optional settings passed to comma or comma specific options csv_options = options.slice(*CSV_HANDLER::DEFAULT_OPTIONS.merge(Comma::DEFAULT_OPTIONS).keys) - csv_options = csv_options.each_with_object({}) do |(k, v), h| - # XXX: Convert string to boolean - h[k] = case k - when :write_headers - (v != 'false') if v.is_a?(String) - else - v - end - end data = obj.to_comma(csv_options) data = "\xEF\xBB\xBF#{data}" if with_bom disposition = "attachment; filename=\"#{filename}.#{extension}\"" diff --git a/lib/comma/generator.rb b/lib/comma/generator.rb index 0c263e10..bfaf8528 100644 --- a/lib/comma/generator.rb +++ b/lib/comma/generator.rb @@ -4,14 +4,10 @@ module Comma class Generator def initialize(instance, style) @instance = instance - @style = style - @options = {} - - return unless @style.is_a?(Hash) - - @options = @style.clone - @style = @options.delete(:style) || Comma::DEFAULT_OPTIONS[:style] - @filename = @options.delete(:filename) + parsed = Comma::Options.parse(style) + @style = parsed[:style] + @filename = parsed[:filename] + @options = parsed[:csv] end def run(iterator_method) diff --git a/lib/comma/options.rb b/lib/comma/options.rb new file mode 100644 index 00000000..6fe83faf --- /dev/null +++ b/lib/comma/options.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Comma + module Options + module_function + + def parse(input) + return { style: input, filename: nil, csv: {} } unless input.is_a?(Hash) + + csv = input.dup + 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) + + { style: style, filename: filename, csv: csv } + end + end +end diff --git a/spec/comma/options_spec.rb b/spec/comma/options_spec.rb new file mode 100644 index 00000000..bd7b846e --- /dev/null +++ b/spec/comma/options_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Comma::Options do # rubocop:disable Metrics/BlockLength + describe '.parse' do # rubocop:disable Metrics/BlockLength + it 'treats a bare symbol as the style, with no filename or csv options' do + expect(described_class.parse(:brief)).to eq(style: :brief, filename: nil, csv: {}) + end + + it 'passes a bare nil through unchanged rather than defaulting it' do + expect(described_class.parse(nil)).to eq(style: nil, filename: nil, csv: {}) + end + + it 'passes a bare false through unchanged rather than defaulting it' do + expect(described_class.parse(false)).to eq(style: false, filename: nil, csv: {}) + end + + it 'extracts :style from a hash, defaulting to :default when absent' do + result = described_class.parse(col_sep: ';') + expect(result[:style]).to eq(:default) + expect(result[:csv]).to eq(col_sep: ';') + end + + it 'extracts an explicit :style from a hash' do + result = described_class.parse(style: :brief, col_sep: ';') + expect(result[:style]).to eq(:brief) + expect(result[:csv]).to eq(col_sep: ';') + end + + it 'extracts :filename from a hash and excludes it from :csv' do + result = described_class.parse(filename: 'export.csv', col_sep: ';') + expect(result[:filename]).to eq('export.csv') + expect(result[:csv]).to eq(col_sep: ';') + end + + it 'passes through a real boolean write_headers unchanged' do + result = described_class.parse(write_headers: false) + expect(result[:csv]).to eq(write_headers: false) + end + + it 'coerces the string "false" for write_headers to boolean false' do + result = described_class.parse(write_headers: 'false') + expect(result[:csv]).to eq(write_headers: false) + end + + it 'coerces any other string for write_headers to boolean true' do + result = described_class.parse(write_headers: 'true') + expect(result[:csv]).to eq(write_headers: true) + end + + it 'does not mutate the input hash' do + input = { style: :brief, filename: 'f.csv', col_sep: ';' } + described_class.parse(input) + expect(input).to eq(style: :brief, filename: 'f.csv', col_sep: ';') + end + + it 'does not raise when given a frozen hash' do + input = { style: :brief, col_sep: ';' }.freeze + expect { described_class.parse(input) }.not_to raise_error + end + end +end