[#2524] Add new --author-dedup-mode flag for deduplicating authors#2532
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new --author-dedup-mode flag that allows users to deduplicate authors based on author-config.csv while displaying all commit authors in the report. This addresses issue #2524, which identified that the current approach requires listing ALL authors in author-config.csv even when only one author needs deduplication.
Changes:
- Added
--author-dedup-modeCLI flag that enables author deduplication based on configured aliases - Modified
ReportGenerator.updateAuthorList()to add all commit authors when dedup mode is enabled, skipping those that match configured aliases - Added documentation for the new flag in the CLI reference guide
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/reposense/parser/ArgsParser.java | Adds CLI flag definition and parsing logic for --author-dedup-mode |
| src/main/java/reposense/model/CliArguments.java | Adds isAuthorDedupMode field, getter, and builder method; updates equals() method |
| src/main/java/reposense/model/RepoConfiguration.java | Adds isAuthorDedupMode field, setter/getter, and static configuration method; includes formatting fixes for parameter alignment |
| src/main/java/reposense/RepoSense.java | Propagates the isAuthorDedupMode flag to repo configurations |
| src/main/java/reposense/report/ReportGenerator.java | Implements core deduplication logic in updateAuthorList() method |
| src/test/java/reposense/parser/ArgsParserTest.java | Adds tests to verify the CLI flag is parsed correctly |
| docs/ug/cli.md | Documents the new flag with usage instructions and requirements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -335,7 +342,8 @@ public static CliArguments parse(String[] args) throws HelpScreenException, Pars | |||
| .originalityThreshold(originalityThreshold) | |||
| .isPortfolio(isPortfolio) | |||
| .isFreshClonePerformed(shouldPerformFreshCloning) | |||
| .isOnlyTextRefreshed(shouldRefreshOnlyText); | |||
| .isOnlyTextRefreshed(shouldRefreshOnlyText) | |||
| .isAuthorDedupMode(isAuthorDedupMode); | |||
There was a problem hiding this comment.
There's no validation to ensure that the --author-dedup-mode flag is used correctly. According to the documentation, this flag "must be used in conjunction with the --config flag and requires an author-config.csv file to be present." However, the code doesn't enforce this requirement.
If a user specifies --author-dedup-mode with --repos (instead of --config), or if they use --author-dedup-mode with --config but no author-config.csv exists, the flag will be silently ignored without any warning or error message. This could lead to user confusion.
Consider adding validation in ArgsParser.parse() to check if isAuthorDedupMode is true, and if so, verify that:
- configFolderPath is not the default path (or that --config was explicitly provided)
- Optionally, check if the author-config.csv file exists and warn if it doesn't
Alternatively, you could add a warning log in ReportGenerator.updateAuthorList() when isAuthorDedupMode is true but the conditions aren't met.
There was a problem hiding this comment.
@yizhong187 agreed, and i think it should just be limited to a warning. meaning if dedup flag is used but config flag is absent or config csv is absent, you can consider displaying a warning saying the dedup flag will be ignored. giving an error might be too much
| @Test | ||
| public void parse_withAuthorDedupMode_success() throws Exception { | ||
| String input = new InputBuilder() | ||
| .addRepos(TEST_REPO_REPOSENSE, TEST_REPO_BETA) | ||
| .add("--author-dedup-mode") | ||
| .build(); | ||
| CliArguments cliArguments = ArgsParser.parse(translateCommandline(input)); | ||
|
|
||
| Assertions.assertTrue(cliArguments.isAuthorDedupMode()); | ||
| } | ||
|
|
||
| @Test | ||
| public void parse_withoutAuthorDedupMode_success() throws Exception { | ||
| String input = new InputBuilder().addRepos(TEST_REPO_REPOSENSE, TEST_REPO_BETA).build(); | ||
| CliArguments cliArguments = ArgsParser.parse(translateCommandline(input)); | ||
|
|
||
| Assertions.assertFalse(cliArguments.isAuthorDedupMode()); | ||
| } |
There was a problem hiding this comment.
The test coverage for the author deduplication mode is insufficient. While these tests verify that the CLI flag is parsed correctly, there are no integration tests that verify the core deduplication logic in ReportGenerator.updateAuthorList().
Consider adding tests that verify:
- When author-config.csv contains configured authors with aliases, those aliases are correctly deduplicated
- Authors not in author-config.csv are added as new authors
- The interaction between configured authors and commit authors works as expected
Looking at RepoConfigurationTest.java, there's a pattern for testing updateAuthorList() using reflection (see the test repoConfig_removeIgnoredAuthors_success). A similar test should be added for author dedup mode to ensure the feature works correctly end-to-end.
There was a problem hiding this comment.
@yizhong187 might be out of scope - i dont see tests for the originality threshold config flag, for example. at the very least if you want to add tests for this feature, it shouldn't be part of the ParserTest file
CYX22222003
left a comment
There was a problem hiding this comment.
LGMT in general, just some small nitpicks to fix.
There was a problem hiding this comment.
Run the system tests locally on Windows. LGTM.
|
The following links are for previewing this pull request:
|
Fixes #2524
Proposed commit message
Manual Testing
author-config.csvinto the config folder with the below content:repo-config.csvinto the config folder with the below content:./gradlew run -Dargs="--config config --view --since 1/1/2025 --until 31/1/2026 --author-dedup-mode"