Skip to content

Added group mode to CsvConcat.#665

Merged
skxeve merged 1 commit intomasterfrom
feature/add-group-option-csv-concat
Apr 2, 2026
Merged

Added group mode to CsvConcat.#665
skxeve merged 1 commit intomasterfrom
feature/add-group-option-csv-concat

Conversation

@skxeve
Copy link
Copy Markdown
Collaborator

@skxeve skxeve commented Apr 1, 2026

Brief

Added group mode to CsvConcat.
It uses regex to group files by their naming conventions and merges them group-by-group.

Points to Check

  • Any unexpected impact
  • The overall quality is trending positively

Test

Confirmed

Pass ALL unit test.

Review Limit

  • Write review limit on pull request title.

@skxeve skxeve requested review from Tsuyoposon and x-chi April 1, 2026 13:42
@skxeve skxeve self-assigned this Apr 1, 2026
@skxeve skxeve added the feat A new feature label Apr 1, 2026
@Tsuyoposon
Copy link
Copy Markdown
Collaborator

[imo] Handling both modes in one class makes the validation logic quite complex, so it might be cleaner to split this into two separate classes (like CsvConcat and CsvGroupConcat). We can share the core logic via a base class, which will keep the Pydantic schemas much simpler and more maintainable.

Copy link
Copy Markdown
Collaborator

@Tsuyoposon Tsuyoposon left a comment

Choose a reason for hiding this comment

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

My comments are just suggestions, so I'm approving this.

@skxeve
Copy link
Copy Markdown
Collaborator Author

skxeve commented Apr 2, 2026

@Tsuyoposon
I see your point, and I agree that splitting the classes could simplify the validation logic.

However, in cliboa, it has been a long-standing practice to consolidate related modes into a single class rather than splitting them (e.g., #665), and I implemented this feature in accordance with that policy.
I believe maintaining consistency with the project’s architectural style is important for overall readability.
Furthermore, from a library development perspective, this approach is quite reasonable as it helps prevent an unnecessary proliferation of classes.

In my view, the complexity of the arguments in CsvConcat stems more from the existence of src_filenames.
This parameter is redundant because we have src_pattern, and it is a unique implementation for CsvConcat that doesn't align with the standard FileRead (FileBaseTransform) ecosystem. While it's unclear why it was originally added, deprecating it would significantly simplify the argument patterns.

Since this PR is already approved and deprecation is out of scope for this task, I will proceed with the merge as is. I'll make sure to open a separate issue for the deprecation so we don't lose track of it.

@skxeve skxeve merged commit ea0dfe1 into master Apr 2, 2026
10 checks passed
@skxeve skxeve deleted the feature/add-group-option-csv-concat branch April 2, 2026 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat A new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants