Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Console application for filtering CSV #321

Draft
wants to merge 23 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
02a733d
Console application for filtering CSV
BurdetteLamar Nov 18, 2024
c23e371
Update test/csv/csv-filter/test_csv_filter.rb
BurdetteLamar Dec 16, 2024
f6a7b29
Update test/csv/csv-filter/test_csv_filter.rb
BurdetteLamar Dec 16, 2024
e929d19
Update test/csv/csv-filter/test_csv_filter.rb
BurdetteLamar Dec 16, 2024
c6f9dd6
Respond to review
BurdetteLamar Dec 16, 2024
6a01bbf
Respond to review
BurdetteLamar Dec 16, 2024
8fee505
Update test/csv/csv-filter/test_csv_filter.rb
BurdetteLamar Dec 17, 2024
c354176
Update test/csv/csv-filter/test_csv_filter.rb
BurdetteLamar Dec 17, 2024
97444e7
Update test/csv/csv-filter/test_csv_filter.rb
BurdetteLamar Dec 17, 2024
7102e61
Update test/csv/csv-filter/test_csv_filter.rb
BurdetteLamar Dec 17, 2024
cdc2b47
Respond to review
BurdetteLamar Dec 17, 2024
119e98c
Respond to review
BurdetteLamar Dec 18, 2024
ebb8f48
Respond to review
BurdetteLamar Dec 18, 2024
1776b21
Update test/csv/csv-filter/test_csv_filter.rb
BurdetteLamar Feb 8, 2025
c3e960e
Update test/csv/csv-filter/test_csv_filter.rb
BurdetteLamar Feb 8, 2025
0658a05
Update test/csv/csv-filter/test_csv_filter.rb
BurdetteLamar Feb 8, 2025
b5cdca6
Update test/csv/csv-filter/test_csv_filter.rb
BurdetteLamar Feb 8, 2025
ae72bb6
Update test/csv/csv-filter/test_csv_filter.rb
BurdetteLamar Feb 8, 2025
a63628d
Update test/csv/csv-filter/test_csv_filter.rb
BurdetteLamar Feb 8, 2025
b176c63
Update test/csv/csv-filter/test_csv_filter.rb
BurdetteLamar Feb 8, 2025
60ff670
Update test/csv/csv-filter/test_csv_filter.rb
BurdetteLamar Feb 8, 2025
31c2f2f
Update test/csv/csv-filter/test_csv_filter.rb
BurdetteLamar Feb 8, 2025
2d67ff1
Respond to review
BurdetteLamar Feb 8, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions bin/csv-filter
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#!/usr/bin/env ruby

require 'optparse'
require 'csv'

options = {}

parser = OptionParser.new

parser.version = CSV::VERSION
parser.banner = <<-BANNER
Usage: #{parser.program_name} [options]

Reads and parses the CSV text content of the standard input per the given input options.
From that content, generates CSV text per the given output options
and writes that text to the standard output.

BANNER

parser.separator('Generic Options')
parser.separator(nil)

parser.on('-h', '--help', 'Prints this help.') do
puts parser
exit
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this?
--help will work by default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.


parser.on('-v', '--version', 'Prints version.') do
puts CSV::VERSION
exit
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this? csv-filter --version will work because we have parser.version = CSV::VERSION.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.


parser.parse!

CSV.filter(**options) do |row|
end
179 changes: 179 additions & 0 deletions test/csv/csv-filter/test_csv_filter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,179 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this for newly created file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

# frozen_string_literal: false

require_relative '../helper'

require 'csv'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove this because ../helper has this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.


class TestFilter < Test::Unit::TestCase

# Some rows data (useful as default).
Rows = [
%w[aaa bbb ccc],
%w[ddd eee fff],
]

def setup
# In case the previous test left this as true.
$TEST_DEBUG = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

end

# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.


# Return the test name (retrieved from the call stack).
def get_test_name
caller.each do |x|
method_name = x.split(' ').last.gsub(/\W/, '')
return method_name if method_name.start_with?('test')
end
raise RuntimeError.new('No test method name found.')
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use Test::Unit::TestCase#name, Test::Unit::TestCase#local_name or Test::Unit::TestCase#method_name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.


# Perform the testing defined in the caller's block.
def do_test(debugging: false)
# Just the caller's block, unless debugging.
unless debugging
yield
return
end
# Wrap the caller's block with debugging information.
$TEST_DEBUG = true
test_name = get_test_name
debug('BEGIN', test_name, newline: true)
yield
debug('END', test_name)
$TEST_DEBUG = false
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.


# Return CSV string generated from rows array and options.
def make_csv_s(rows: Rows, **options)
csv_s = CSV.generate(**options) do|csv|
rows.each do |row|
csv << row
end
end
csv_s
end

# Return filepath of file containing CSV data.
def csv_filepath(csv_in_s, dirpath, option_sym)
filename = "#{option_sym}.csv"
filepath = File.join(dirpath, filename)
File.write(filepath, csv_in_s)
filepath
end

# Return stdout and stderr from CLI execution.
def execute_in_cli(filepath, cli_options_s = '')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • How about renaming this to run_csv_filter or something?
  • Why do we need to use string not array for options?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed; modified to use array.

debug('cli_options_s', cli_options_s)
command = "cat #{filepath} | ruby bin/csv-filter #{cli_options_s}"
capture_subprocess_io do
system(command)
end
end

# Return results for CLI-only option (or invalid option).
def results_for_cli_option(option_name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use better name? This executes csv-filter not a simple getter. So we should use a verb not a noun (results) for method name. (I think that we can unify this and run_csv_filter and use run_csv_filter for it...)

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 = execute_in_cli(filepath, option_name)
end
[cli_out_s, cli_err_s]
end

# Get and return the actual output from the API.
def get_via_api(csv_in_s, **api_options)
cli_out_s = ''
CSV.filter(csv_in_s, cli_out_s, **api_options) {|row| }
cli_out_s
end

# Test for invalid option.

def test_invalid_option
do_test(debugging: false) do
%w[-Z --ZZZ].each do |option_name|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to test both of short option and long option?
This is not a OptionParser test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified.

cli_out_s, cli_err_s = results_for_cli_option(option_name)
assert_empty(cli_out_s)
assert_match(/OptionParser::InvalidOption/, cli_err_s)
end
end
end

# Test for no options.

def test_no_options
do_test(debugging: false) do
csv_in_s = make_csv_s
cli_out_s = get_via_api(csv_in_s)
assert_equal(csv_in_s, cli_out_s)
end
end

# Tests for general options.

def test_option_h
do_test(debugging: false) do
%w[-h --help].each do |option_name|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified.

cli_out_s, cli_err_s = results_for_cli_option(option_name)
assert_match(/Usage/, cli_out_s)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified.

assert_empty(cli_err_s)
end
end
end

def test_option_v
do_test(debugging: false) do
%w[-v --version].each do |option_name|
cli_out_s, cli_err_s = results_for_cli_option(option_name)
assert_match(/\d+\.\d+\.\d+/, cli_out_s)
assert_empty(cli_err_s)
end
end
end

# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.


end
Loading