feat: add keywords CLI tool for text vectorization (#122)#163
Conversation
Greptile SummaryThis PR adds a
Confidence Score: 3/5Safe to merge after addressing the unclosed file handles in command_fit; the rest of the changes are additive and well-tested. The command_fit path calls File.open for each matched file and never closes those handles — on large corpora or under error conditions the descriptors stay open until GC collects them. The number_with_delimiter typo is a real defect in the method signature. Everything else (MultiIO, TFIDF attr_reader additions, tests) is straightforward and correct. lib/classifier/keywords/cli.rb — specifically the file-handle management in command_fit and the keyword argument typo in number_with_delimiter. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[exe/keywords: ARGV] --> B[CLI.new.run]
B --> C[parse_options]
C -->|--help / --version| D[throw :done → output result]
C --> E[execute_command]
E -->|exit_code != 0 or output present| F[return early]
E -->|args.first == 'fit'| G[command_fit]
E -->|args.first == 'extract'| H[command_extract]
E -->|args.first == 'info'| I[command_info]
E -->|else| J[command_keywords]
G -->|args empty| G1[stdin / $stdin]
G -->|args present| G2[Dir.glob → File.open streams]
G1 & G2 --> G3[TFIDF#fit_from_stream via MultiIO]
G3 --> G4[tfidf.save_to_file]
H -->|args empty| H1[stdin / $stdin.read]
H -->|file exists| H2[File.read]
H -->|arg not a file| H3[treat arg as literal text]
H1 & H2 & H3 --> H4[transform → @output]
I --> I1[TFIDF.load_from_file → format stats → @output]
J -->|no args, no stdin, tty?| J1[show_getting_started]
J -->|otherwise| J2[stdin / @args.join → transform → @output]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[exe/keywords: ARGV] --> B[CLI.new.run]
B --> C[parse_options]
C -->|--help / --version| D[throw :done → output result]
C --> E[execute_command]
E -->|exit_code != 0 or output present| F[return early]
E -->|args.first == 'fit'| G[command_fit]
E -->|args.first == 'extract'| H[command_extract]
E -->|args.first == 'info'| I[command_info]
E -->|else| J[command_keywords]
G -->|args empty| G1[stdin / $stdin]
G -->|args present| G2[Dir.glob → File.open streams]
G1 & G2 --> G3[TFIDF#fit_from_stream via MultiIO]
G3 --> G4[tfidf.save_to_file]
H -->|args empty| H1[stdin / $stdin.read]
H -->|file exists| H2[File.read]
H -->|arg not a file| H3[treat arg as literal text]
H1 & H2 & H3 --> H4[transform → @output]
I --> I1[TFIDF.load_from_file → format stats → @output]
J -->|no args, no stdin, tty?| J1[show_getting_started]
J -->|otherwise| J2[stdin / @args.join → transform → @output]
Prompt To Fix All With AIFix the following 5 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 5
lib/classifier/keywords/cli.rb:128-134
**File handles opened but never closed**
`File.open(f)` inside `command_fit` creates file descriptors that are never explicitly closed. If `fit_from_stream` raises mid-stream, or if a large number of files is passed, these handles remain open until GC runs. Use `ensure` or restructure to close after use — for example, open, process, then close each stream, or collect the paths and let `MultiIO` open/close them internally.
### Issue 2 of 5
lib/classifier/keywords/cli.rb:246
Typo in the keyword argument name: `delemiter` should be `delimiter`. Even though this is a private method, the misspelling is inconsistent and would be confusing if the argument ever needs to be overridden internally.
```suggestion
def number_with_delimiter(number, delimiter: ',')
```
### Issue 3 of 5
lib/classifier/keywords/cli.rb:125-126
These inline comments just restate what `@args.shift` already communicates through context (the enclosing method is named `command_fit`, `command_extract`, and `command_info`). Per the de-AI review rule, comments that repeat what the code already shows should be removed.
```suggestion
def command_fit
@args.shift
```
In this review, suggest simpl... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
### Issue 4 of 5
lib/classifier/keywords/cli.rb:149-159
Nested conditional in a value assignment. The inner `if File.exist?` inside the `else` branch can be flattened using an early assignment with a guard, making the logic easier to follow at a glance.
```suggestion
if @args.empty?
document = @stdin ? @stdin.to_s : $stdin.read
else
file = File.expand_path(@args.first)
document = File.exist?(file) ? File.read(file) : @args.first
end
```
In this review, suggest simpl... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
### Issue 5 of 5
exe/keywords:4-6
This comment restates exactly what the two `Encoding` assignments already express. Per the de-AI review rule, comments that repeat what the code shows should be removed.
```suggestion
Encoding.default_external = Encoding::UTF_8
Encoding.default_internal = Encoding::UTF_8
```
In this review, suggest simpl... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
Reviews (1): Last reviewed commit: "feat: add keywords CLI tool for text vec..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @output << 'General Options:' | ||
| @output << ' -m, --model FILE Model file (default: ./keywords.json)' | ||
| @output << ' -n, --top N Show top N terms only (e.g. keywords -n 5 "text...")' | ||
| @output << ' -q Quiet mode (clean output for scripting/pipelines)' |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [95/80]
| @output << '' | ||
| @output << 'General Options:' | ||
| @output << ' -m, --model FILE Model file (default: ./keywords.json)' | ||
| @output << ' -n, --top N Show top N terms only (e.g. keywords -n 5 "text...")' |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [98/80]
| @output << ' # Max DF: 1.0' | ||
| @output << '' | ||
| @output << 'General Options:' | ||
| @output << ' -m, --model FILE Model file (default: ./keywords.json)' |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [83/80]
| end | ||
|
|
||
| def show_getting_started | ||
| @output << 'Keywords - Keyword extraction and term analysis using TF-IDF' |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [81/80]
| transform(document) | ||
| end | ||
|
|
||
| def show_getting_started |
There was a problem hiding this comment.
Metrics/AbcSize: Assignment Branch Condition size for show_getting_started is too high. [41/15]
Metrics/MethodLength: Method has too many lines. [41/10]
| end | ||
| end | ||
|
|
||
| def execute_command |
There was a problem hiding this comment.
Metrics/MethodLength: Method has too many lines. [12/10]
|
|
||
| opts.on('--ngram MIN,MAX', Array, 'N-gram range (default: 1,1)') do |range| | ||
| invalid_range = range.count != 2 || !range.all? { |n| n =~ /\d+/ } | ||
| raise OptionParser::InvalidArgument, 'must have only 2 integers' if invalid_range |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [93/80]
| @options[:max_df] = n | ||
| end | ||
|
|
||
| opts.on('--ngram MIN,MAX', Array, 'N-gram range (default: 1,1)') do |range| |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [85/80]
| @options[:min_df] = n | ||
| end | ||
|
|
||
| opts.on('--max-df N', Float, 'Maximum document frequency ratio (default: 1.0)') do |n| |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [96/80]
| @options[:top] = n | ||
| end | ||
|
|
||
| opts.on('--min-df N', Integer, 'Minimum document frequency (default: 1)') do |n| |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [90/80]
| opts.separator '' | ||
| opts.separator 'Options:' | ||
|
|
||
| opts.on('-m', '--model FILE', 'Model file (default: ./keywords.json)') do |file| |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [90/80]
| opts.banner = 'Usage: keywords [text] [options] [command] [arguments]' | ||
| opts.separator '' | ||
| opts.separator 'Commands:' | ||
| opts.separator ' fit <files...> Fit the model from files or stdin' |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [81/80]
| private | ||
|
|
||
| def parse_options | ||
| @parser = OptionParser.new do |opts| |
There was a problem hiding this comment.
Metrics/BlockLength: Block has too many lines. [39/25]
|
|
||
| private | ||
|
|
||
| def parse_options |
There was a problem hiding this comment.
Metrics/AbcSize: Assignment Branch Condition size for parse_options is too high. [33.9/15]
Metrics/MethodLength: Method has too many lines. [44/10]
| rescue StandardError => e | ||
| @error << "Error: #{e.message}" | ||
| @exit_code = 1 | ||
| { output: @output.join("\n"), error: @error.join("\n"), exit_code: @exit_code } |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [87/80]
| # @rbs @exit_code: Integer | ||
| # @rbs @parser: OptionParser | ||
|
|
||
| def initialize(args, stdin: nil) |
There was a problem hiding this comment.
Metrics/MethodLength: Method has too many lines. [13/10]
|
|
||
| module Classifier | ||
| module Keywords | ||
| class CLI |
There was a problem hiding this comment.
Metrics/ClassLength: Class has too many lines. [200/100]
Style/Documentation: Missing top-level class documentation comment.
| @@ -0,0 +1,252 @@ | |||
| # rbs_inline: enabled | |||
There was a problem hiding this comment.
Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
|
|
||
| module Classifier | ||
| module Streaming | ||
| # A utility class that wraps multiple IO-like streams and treats them as a single, |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [86/80]
| @@ -0,0 +1,42 @@ | |||
| # frozen_string_literal: true | |||
| # rbs_inline: enabled | |||
There was a problem hiding this comment.
Layout/EmptyLineAfterMagicComment: Add an empty line after magic comments.
| end | ||
|
|
||
| def test_keywords_without_args | ||
| skip( |
There was a problem hiding this comment.
Style/MultilineIfModifier: Favor a normal unless-statement over a modifier clause in a multiline statement.
| require 'classifier/keywords/cli' | ||
|
|
||
| module Keywords | ||
| class CLITest < Minitest::Test |
There was a problem hiding this comment.
Metrics/ClassLength: Class has too many lines. [122/100]
| require 'classifier/keywords/cli' | ||
|
|
||
| module Keywords | ||
| class CLITest < Minitest::Test |
There was a problem hiding this comment.
Metrics/ClassLength: Class has too many lines. [124/100]
|
@cardmagic can you take a look, please? |
cardmagic
left a comment
cardmagic
left a comment
There was a problem hiding this comment.
Deep review
Solid, well-tested implementation that tracks the issue closely: separate tool, transform-as-default, stdin-friendly, reuses TFIDF, clean command dispatch. MultiIO is minimal and correct, the LineReader integration is sound (it consumes via each_line, and estimate_line_count is guarded so multi-file fit degrades gracefully), rubocop is clean, and CI/typecheck are green. The earlier Greptile items all landed in commit 2. Verified locally by building + installing the gem and driving the CLI through ~25 scenarios.
One blocking issue and a ring of robustness/spec-fidelity gaps:
🔴 Blocking — keywords doesn't install. classifier.gemspec (not touched by this PR) still has s.executables = ['classifier']. exe/keywords ships inside the gem via s.files, but RubyGems only generates a binstub for names in executables. I built and installed the gem into an isolated GEM_HOME: only classifier appears in bin/, no keywords. So after gem install classifier, the tool the issue asks for isn't on PATH. Fix: s.executables = %w[classifier keywords]. The tests miss this because they call Classifier::Keywords::CLI directly instead of the installed binary. (Inline note left on exe/keywords.)
Robustness (details inline): fit silently saves an empty model (exit 0) when a glob matches nothing, when an explicitly-named file doesn't exist, or on empty stdin; --ngram validation accepts garbage like 1abc,2xyz; usage errors split between exit 1 and exit 2; raw Ruby exceptions leak to users (missing model, directory arg, negative -n).
Spec deviations vs #122: -q is effectively a no-op for the primary transform output; output tokens are Porter-stemmed (rubi, eleg, program) rather than the whole words the issue's examples show; fit granularity is one-document-per-line, which drives IDF and diverges from "build vocabulary from files".
Docs: README has a ## Command Line section for classifier but nothing for the new keywords command — it ships undiscoverable.
None of this is architectural; all are small, localized fixes. Minimum before merge: the gemspec executables line and the silent-empty-model guard.
| def mapping_stem_to_word_for_words(words, min_word_length) | ||
| h = {} | ||
| words.map { _1.tap(&:downcase!) }.tally.each do |word, count| | ||
| next unless !CORPUS_SKIP_WORDS.include?(word) && word.length >= min_word_length |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [85/80]
| end | ||
|
|
||
| # @rbs (Array[String], Integer) -> Hash[Symbol, Integer] | ||
| def mapping_stem_to_word_for_words(words, min_word_length) |
There was a problem hiding this comment.
Metrics/AbcSize: Assignment Branch Condition size for mapping_stem_to_word_for_words is too high. [17.97/15]
| word_hash_for_words(gsub(/[^\w\s]/, '').split, min_word_length) | ||
| end | ||
|
|
||
| # Builds a mapping between stemmed roots and their most frequent original words. |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [82/80]
| tfidf = TFIDF.load_from_file(@options[:model]) | ||
| vector = tfidf.transform(document).sort_by { |_, v| v }.reverse | ||
| vector = vector.first(@options[:top]) if @options[:top] | ||
| @output << vector.map { |k, v| "#{stem_map[k]}:#{v.round(2)}" }.join(' ') |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [81/80]
| @output << 'Run "keywords --help" for full usage.' | ||
| end | ||
|
|
||
| def transform(document) |
There was a problem hiding this comment.
Metrics/AbcSize: Assignment Branch Condition size for transform is too high. [17.58/15]
| end | ||
|
|
||
| opts.on('--ngram MIN,MAX', Array, 'N-gram range (default: 1,1)') do |range| | ||
| raise OptionParser::InvalidArgument, 'requires exactly two values' if range.count != 2 |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [98/80]
| end | ||
|
|
||
| opts.on('-n', '--top N', Integer, 'Show top N terms only') do |n| | ||
| raise OptionParser::InvalidArgument, 'must be positive' unless n.positive? |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [86/80]
| opts.banner = 'Usage: keywords [text] [options] [command] [arguments]' | ||
| opts.separator '' | ||
| opts.separator 'Commands:' | ||
| opts.separator ' fit <files...> Fit the model from files or stdin (each line is treated as a separate document)' |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [127/80]
| private | ||
|
|
||
| def parse_options | ||
| @parser = OptionParser.new do |opts| |
There was a problem hiding this comment.
Metrics/BlockLength: Block has too many lines. [42/25]
|
|
||
| private | ||
|
|
||
| def parse_options |
There was a problem hiding this comment.
Metrics/AbcSize: Assignment Branch Condition size for parse_options is too high. [40.61/15]
Metrics/CyclomaticComplexity: Cyclomatic complexity for parse_options is too high. [7/6]
Metrics/MethodLength: Method has too many lines. [47/10]
3 participants
Closes #122