Deduplicate column argument parsing between extractors - #174
Open
eitoball wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors Comma’s CSV DSL execution by centralizing the column-argument shape parsing ((none), String, Symbol, Hash) into Comma::Extractor#method_missing, while leaving per-column resolution (data vs. header) to subclass hooks. This reduces duplicated branching logic between the data and header extractors without changing the public DSL.
Changes:
- Moved argument-shape branching for column definitions into
lib/comma/extractor.rband delegated resolution via anextract_columnhook. - Simplified
DataExtractorandHeaderExtractorto implementextract_column+column_kind(and removed their duplicatedmethod_missing). - Updated
.rubocop_todo.ymlexclusions to reflectmethod_missingrelocating toExtractor.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/comma/extractor.rb | Centralizes column argument parsing in method_missing and delegates resolution via extract_column. |
| lib/comma/data_extractor.rb | Removes duplicated method_missing; adds extract_column implementation for value extraction. |
| lib/comma/header_extractor.rb | Removes duplicated method_missing; adds extract_column implementation for header humanization. |
| .rubocop_todo.yml | Updates Style/MissingRespondToMissing excludes to match the new method_missing location. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
DataExtractor#method_missing and HeaderExtractor#method_missing implemented identical branching over argument shapes (none/String/ Symbol/Hash). Move that branching into Extractor#method_missing and have each subclass supply only an extract_column hook (value vs. label resolution) and a column_kind label for error messages. Fixes #162
eitoball
force-pushed
the
refactor/162-dedupe-extractor-column-parsing
branch
from
August 29, 2026 10:31
e9c0c75 to
22603ed
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Move the shared column-argument branching logic out of
DataExtractor#method_missingandHeaderExtractor#method_missingand into a single implementation onComma::Extractor.Why
Both extractors implemented nearly identical branching over argument shapes (none /
String/Symbol/Hash), differing only in how each column is resolved (extract a value vs. build a header label). This duplication meant bug fixes and new column types required parallel changes in two places.Changes
lib/comma/extractor.rb:method_missingnow owns the argument-shape branching (blank /Hash/Symbol/String) and delegates per-column resolution to anextract_columnhook, usingcolumn_kindfor error messages.lib/comma/data_extractor.rb: removed the duplicatedmethod_missing; now only implementsextract_column(resolves a value viaExtractValueFromInstance/ExtractValueFromAssociationOfInstance) andcolumn_kind.lib/comma/header_extractor.rb: removed the duplicatedmethod_missing; now only implementsextract_column(resolves a label viavalue_humanizer) andcolumn_kind..rubocop_todo.yml: updatedStyle/MissingRespondToMissingexclusions to reflectmethod_missingmoving toextractor.rb(still excluded ondata_extractor.rbfor its unrelatednull_associationhelper).No change to the public DSL. All existing specs in
data_extractor_spec.rb,header_extractor_spec.rb, andcomma_spec.rbpass unchanged.Fixes #162