feat: support multiple dependency files in config and cli#334
Conversation
sdn4z
force-pushed
the
multiple-dep-file-cli
branch
from
3d53e3b to
2d0d3cf
Compare
|
/lgtm review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🦉 lgtm Review
Score: Needs Work 🔧
🔍 Summary
Thank you for adding support for multiple dependency files. This is a valuable feature.
The overall approach is good, but there are a couple of critical bugs in the current implementation that need to be fixed. Specifically, the results are only being returned for the last file processed, and parsing a single dependency file from the config is broken. There's also a performance consideration with how package sources are fetched.
Additionally, the test suite needs to be expanded to cover these new multi-file scenarios, which would have helped catch these issues.
I've left detailed comments on each point. After addressing these, the PR should be in good shape. I'm scoring it 3/5 for now.
More information
- Id:
b94f449adb294b34b8ce29c62ea7963e - Model:
gemini-2.5-pro - Created at:
2025-09-16T15:50:49.412248+00:00
Usage summary
- Request count:
2 - Request tokens:
54,358 - Response tokens:
22,183 - Total tokens:
76,541
See the 📚 lgtm-ai repository for more information about lgtm.
| @@ -36,21 +36,21 @@ class TestCheckDependencies: | |||
| ( | |||
There was a problem hiding this comment.
🦉 🧪 Testing
Severity: MEDIUM 🟡
The PR adds support for multiple dependency files, but there are no tests to verify this new functionality end-to-end. Please add a test case where check_dependencies is called with two or more dependency files, each containing typos, and assert that the results contain typos from all provided files. Such a test would have likely caught the bug with typos_by_file being re-initialized within the loop.
More information about this comment
- File:
tests/main/test_main.py - Line:
1 - Relative line:
1 - With suggestion:
No - Suggestion ready for replacement:
No
| @@ -1,7 +1,6 @@ | |||
| import dataclasses | |||
There was a problem hiding this comment.
🦉 🧪 Testing
Severity: MEDIUM 🟡
There is no test case that verifies the configuration parsing when dependency_file is a single string in the TOML file. The existing tests were updated to use a list of files, but the case of a single string value should also be covered to prevent regressions and catch issues like the one in _get_read_config. Please add a test case for this scenario.
More information about this comment
- File:
tests/config/test_config_handler.py - Line:
1 - Relative line:
1 - With suggestion:
No - Suggestion ready for replacement:
No
| for dep_file in dependency_files: | ||
| dependency_managers = _get_dependency_managers_and_parsers_mapping(dep_file) | ||
| typos_by_file = TyposquatCheckResults() |
There was a problem hiding this comment.
🦉 🎯 Correctness
Severity: HIGH 🔴
typos_by_file = TyposquatCheckResults()typos_by_file is re-initialized inside the for dep_file in dependency_files: loop. This will cause the function to only return the results from the last dependency file that is processed. It should be initialized before the loop.
| for dep_file in dependency_files: | |
| dependency_managers = _get_dependency_managers_and_parsers_mapping(dep_file) | |
| typos_by_file = TyposquatCheckResults() | |
| typos_by_file = TyposquatCheckResults() | |
| for dep_file in dependency_files: | |
| dependency_managers = _get_dependency_managers_and_parsers_mapping(dep_file) |
More information about this comment
- File:
src/twyn/main.py - Line:
167 - Relative line:
60 - With suggestion:
Yes - Suggestion ready for replacement:
Yes
1 participant
Support multiple dependency files, either via the cli or via the config
refs #325