Add bare bones console application for filtering CSV#321
Add bare bones console application for filtering CSV#321kou merged 27 commits intoruby:masterfrom BurdetteLamar:csv-filter
Conversation
bin/csv-filter
Outdated
| parser.on('-v', '--version', 'Prints version.') do | ||
| puts CSV::VERSION | ||
| exit | ||
| end |
There was a problem hiding this comment.
Do we need this? csv-filter --version will work because we have parser.version = CSV::VERSION.
bin/csv-filter
Outdated
| parser.on('-h', '--help', 'Prints this help.') do | ||
| puts parser | ||
| exit | ||
| end |
There was a problem hiding this comment.
Do we need this?
--help will work by default.
| @@ -0,0 +1,179 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
We don't need this for newly created file.
| # In case the previous test left this as true. | ||
| $TEST_DEBUG = false |
| # Print debugging information if indicated. | ||
| def debug(label, value, newline: false) | ||
| return unless $TEST_DEBUG | ||
| print("\n") if newline | ||
| printf("%15s: %s\n", label, value.inspect) | ||
| end |
|
|
||
| def test_invalid_option | ||
| do_test(debugging: false) do | ||
| %w[-Z --ZZZ].each do |option_name| |
There was a problem hiding this comment.
Why do we need to test both of short option and long option?
This is not a OptionParser test.
|
|
||
| def test_option_h | ||
| do_test(debugging: false) do | ||
| %w[-h --help].each do |option_name| |
There was a problem hiding this comment.
Could you avoid using each to run multiple tests?
Could you define separated tests instead of checking multiple items in one test? It's for easy to debug on error.
def test_option_h
end
def test_option_help
end| do_test(debugging: false) do | ||
| %w[-h --help].each do |option_name| | ||
| cli_out_s, cli_err_s = results_for_cli_option(option_name) | ||
| assert_match(/Usage/, cli_out_s) |
There was a problem hiding this comment.
assert_equal is better than assert_match on error because assert_equal shows diff too.
assert_equal("Usage: csv-filter [options]\n", cli_out_s.lines.first)| # Two methods copied from module Minitest::Assertions. | ||
| # because we need access to the subprocess io. | ||
|
|
||
| def _synchronize # :nodoc: | ||
| yield | ||
| end | ||
|
|
||
| def capture_subprocess_io | ||
| _synchronize do | ||
| begin | ||
| require "tempfile" | ||
|
|
||
| captured_stdout, captured_stderr = Tempfile.new("out"), Tempfile.new("err") | ||
|
|
||
| orig_stdout, orig_stderr = $stdout.dup, $stderr.dup | ||
| $stdout.reopen captured_stdout | ||
| $stderr.reopen captured_stderr | ||
|
|
||
| yield | ||
|
|
||
| $stdout.rewind | ||
| $stderr.rewind | ||
|
|
||
| return captured_stdout.read, captured_stderr.read | ||
| ensure | ||
| $stdout.reopen orig_stdout | ||
| $stderr.reopen orig_stderr | ||
|
|
||
| orig_stdout.close | ||
| orig_stderr.close | ||
| captured_stdout.close! | ||
| captured_stderr.close! | ||
| end | ||
| end | ||
| end |
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
|
Thanks, @kou, for your careful review; I learned a lot. |
|
@kou, okay to add an option or two? |
|
Ah, let's add options after this is merged for easy to review this PR. |
Works for me! |
|
|
||
| require_relative '../helper' | ||
|
|
||
| require 'csv' |
There was a problem hiding this comment.
Could you remove this because ../helper has this?
| # Return results for CLI-only option (or invalid option). | ||
| def results_for_cli_option(option_name) | ||
| cli_out_s = '' | ||
| cli_err_s = '' | ||
| Dir.mktmpdir do |dirpath| | ||
| sym = option_name.to_sym | ||
| filepath = csv_filepath('', dirpath, sym) | ||
| cli_out_s, cli_err_s = run_csv_filter(filepath, [option_name]) | ||
| end | ||
| [cli_out_s, cli_err_s] | ||
| end |
There was a problem hiding this comment.
Can we remove this?
Can run_csv_filter accept CSV in string as the first argument?
There was a problem hiding this comment.
The tests that use this have CLI-only options (-v -h etc.) that can't be passed to the API. I think we need to keep this.
| # Return CSV string generated from rows array and options. | ||
| def make_csv_s(rows: Rows, **options) | ||
| CSV.generate(**options) do|csv| | ||
| rows.each do |row| | ||
| csv << row | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Do we need this?
How about keeping all CSV data as String not [[...], [...], ...] in this file?
I think that interface of csv-filter is CSV string not a CSV Ruby object.
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
|
@kou, no longer interested in this? |
| def test_invalid_option | ||
| output, error = results_for_cli_option("-Z") | ||
| assert_equal("", output) | ||
| assert_match(/OptionParser::InvalidOption/, error) |
There was a problem hiding this comment.
Can we show --help for an invalid option?
| def test_no_options | ||
| output = api_output(@input) | ||
| assert_equal(@input, output) | ||
| end |
There was a problem hiding this comment.
Is this meaningful test?
I think that we need to use results_for_cli_option not api_output here to test csv-filter.
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
|
@kou, I have merged the suggested changes (but have not yet worked on the other comments). I've restored Also, I could not make the test for option |
|
Can I push some commits to this branch? |
Yes |
|
I've simplified the current implementation. Could you review this? |
BurdetteLamar
left a comment
BurdetteLamar
left a comment
There was a problem hiding this comment.
Looks good to me!
kou
changed the title
|
Thanks. |
2 participants
This is the bare bones (no input/output options yet). Options to be added piecemeal.
The help text is a little misleading; it mentions input and output options, but there are none (yet).