CLI for filter #320

BurdetteLamar · 2024-11-04T19:12:08Z

Adds a command-line interface for CSV::filter; this supports using CSV without having to write Ruby code.

In its present form, this is a true filter: reads from stdin, writes to stdout. More could be done: input could be from ARGF or an IO stream; output could be to IO stream.

BurdetteLamar · 2024-11-04T19:46:07Z

I'm not sure what to do about these failures. I chose to require 'minitest/autorun' because it has the assertion method capture_subprocess_io, which I've used to evaluate the output.

Suggestions welcome.

kou

This is an interested suggestion. But filter is too generic for command line name to handle only CSV.

How about adding csv- prefix or something?

kou · 2024-11-05T04:58:49Z

test/csv/filter/test_filter.rb

+# -*- coding: utf-8 -*-
+# frozen_string_literal: false
+
+require 'minitest/autorun'


We're using test-unit not minitest.

BurdetteLamar · 2024-11-05T17:19:59Z

This is an interested suggestion. But filter is too generic for command line name to handle only CSV.

How about adding csv- prefix or something?

Changed to csv-filter.

BurdetteLamar · 2024-11-05T17:20:51Z

Should have added earlier: I've needed substantial debugging code to get this work done, and have left it in (just in case).

kou

I didn't review all changes. Because there are many parts I need to add comments... Can we start from a smaller script instead of this?

kou · 2024-11-14T05:58:25Z

bin/csv-filter

+
+options = {}
+
+OptionParser.new do |p|


Could you use the following style instead of OptionParser.new do end.parse!? The .parse! part in OptionParser.new do end.parse! is easy to miss.

parser = OptionParser.new parser.program_name = ... parser.on(...) parser.parse!

kou · 2024-11-14T05:59:14Z

bin/csv-filter

+
+OptionParser.new do |p|
+
+  p.program_name = File.basename $0


We can omit this. We can use the default value.

kou · 2024-11-14T05:59:36Z

bin/csv-filter

+
+  p.program_name = File.basename $0
+  p.version = CSV::VERSION
+  p.release = nil


We can omit this.

kou · 2024-11-14T06:00:07Z

bin/csv-filter

+  p.program_name = File.basename $0
+  p.version = CSV::VERSION
+  p.release = nil
+  p.summary_indent = ' ' * 4


We can omit this.

kou · 2024-11-14T06:00:55Z

bin/csv-filter

+  p.version = CSV::VERSION
+  p.release = nil
+  p.summary_indent = ' ' * 4
+  p.banner = <<-EOF


Could you use BANNER not EOF for here document mark?
EOF (End Of File?) is difficult to understand.

kou · 2024-11-14T06:15:26Z

bin/csv-filter

+
+# Returns an array of converter names.
+# \String +s+ contains a comma-separated list of converter names;
+# each optionally begins with a colon.


Why do we need this feature?

kou · 2024-11-14T06:16:31Z

bin/csv-filter

+def parse_converters(s)
+  converters = []
+  s.split(',').each do |name|
+    name.sub(/^:/, '')
+    sym = name.to_sym
+    converters.push(sym)
+  end
+  converters
+end


Suggested change

def parse_converters(s)

converters = []

s.split(',').each do |name|

name.sub(/^:/, '')

sym = name.to_sym

converters.push(sym)

end

converters

end

def parse_converters(raw_converters)

raw_converters.split(",").collect(&:to_sym)

end

Replaced and renamed.

kou · 2024-11-14T06:17:04Z

bin/csv-filter

+  p.on('--field_size_limit N',
+       'Maximum field size (characters)') do |value|
+    options[:field_size_limit] = value.to_i


Suggested change

p.on('--field_size_limit N',

'Maximum field size (characters)') do |value|

options[:field_size_limit] = value.to_i

p.on('--field_size_limit N', Integer,

'Maximum field size (characters)') do |value|

options[:field_size_limit] = value

kou · 2024-11-14T06:17:50Z

bin/csv-filter

+       'Treat the first input row as headers;',
+       'STR is true or comma-separated headers.'
+       ) do |value|
+    options[:headers] = value == 'true' ? true : parse_converters(value)


It's strange that we use parse_converters for headers.
We need better name than parse_converters.

kou · 2024-11-14T06:19:54Z

bin/csv-filter

+  end
+  p.on('--liberal_parsing',
+       'Attempt to parse non-conformant input.') do
+    options[:liberal_parsing] = :no_argument


:no_argument is strange. :liberal_parsing must be true or false: p.on('--[no-]liberal-parsing', ...)

Symbol :no_argument is only internal, and tells the testing how to behave. The CSV user does not see it.

BurdetteLamar · 2024-11-14T11:17:52Z

Thanks, @kou, for your interest, and for this helpful review. I'm at RubyConf in Chicago until the weekend, but will work on this when I get home.

When we're done with this review cycle, I'll be interested in adding to this work. The filter will not be very useful without a config file (maybe similar to ~/.gitconfig?) that will avoid the need for including lots of options on the command line.

BurdetteLamar · 2024-11-17T20:10:00Z

I have commented out 13 of the options (in bin and test), leaving 16 in. Each of those left in is either because it's mentioned in the help or because it has comments here.

Maybe we can work with these, and do the rest piecemeal?

kou · 2024-11-17T23:57:54Z

Could you remove them from this PR instead of commenting them out?

FYI: You can copy the current branch content:

git clone git@github.com:BurdetteLamar/csv.git
cd csv
git switch filter
git switch -c filter-keep
git push origin filter-keep

BTW, can remove more options from this PR to make this script smaller?

BurdetteLamar · 2024-11-18T03:57:48Z

smaller

I'm thinking to close this PR and start fresh, with just --help and --version. Then we can have a new PR for each option as we go, adjusting the help text and version accordingly. Does that work for you?

kou · 2024-11-18T04:48:47Z

Yes. It's better than the current PR.

BurdetteLamar · 2024-11-18T09:28:53Z

Closing this; will begin again with a new PR.

CLI for filter

cc50dd5

BurdetteLamar added the enhancement label Nov 4, 2024

BurdetteLamar requested review from kou and peterzhu2118 November 4, 2024 19:12

kou reviewed Nov 5, 2024

View reviewed changes

BurdetteLamar added 2 commits November 5, 2024 11:11

Adapt to test-unit

70890da

Change executable name from filter to csv-filter

4dac27f

BurdetteLamar requested a review from kou November 6, 2024 14:02

kou reviewed Nov 14, 2024

View reviewed changes

BurdetteLamar added 3 commits November 17, 2024 13:43

Respond to review

9458640

Respond to review

c6a108a

Respond to review

ba9eaff

BurdetteLamar closed this Nov 18, 2024

CLI for filter #320

CLI for filter #320

Conversation

BurdetteLamar commented Nov 4, 2024

Uh oh!

BurdetteLamar commented Nov 4, 2024

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BurdetteLamar commented Nov 5, 2024

Uh oh!

BurdetteLamar commented Nov 5, 2024

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BurdetteLamar commented Nov 14, 2024

Uh oh!

BurdetteLamar commented Nov 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kou commented Nov 17, 2024

Uh oh!

BurdetteLamar commented Nov 18, 2024

Uh oh!

kou commented Nov 18, 2024

Uh oh!

BurdetteLamar commented Nov 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

BurdetteLamar commented Nov 17, 2024 •

edited

Loading