chore: Adds version scanner CI/CD upgrades#17425
Conversation
Adds distinct exit codes, --stdout support, workflow file, and fixes 3.10 truncation.
There was a problem hiding this comment.
Code Review
This pull request introduces negative lookahead patterns to prevent version truncation bugs (e.g., matching 3.10 as 3.1), adds Excel-compatible formatting for matched strings, introduces a --stdout option, and updates exit codes for CI/CD integration. Feedback from the reviewer recommends restricting the Excel-specific wrapping to numeric/version strings to avoid formula errors, using pytest.raises for cleaner test assertions, and removing unused imports and redundant file writes in the stdout logic.
| if matched: | ||
| formatted["matched_string"] = f'="{matched}"' |
There was a problem hiding this comment.
Wrapping every matched string in ="..." is unnecessary for non-numeric strings (like code lines or warning names) and makes the CSV output harder to read in plain text. It can also cause formula errors in Excel if the matched string contains unescaped double quotes.
We should only apply this Excel-specific wrapping to pure numeric/version strings (like 3.10) that Excel would otherwise misinterpret as floats.
| if matched: | |
| formatted["matched_string"] = f'="{matched}"' | |
| if matched and matched.strip().replace('.', '', 1).isdigit(): | |
| formatted["matched_string"] = f'="{matched}"' |
| assert rows[0]["rule_name"] == "python_requires_check" | ||
| assert rows[0]["line_number"] == "1" | ||
| assert rows[0]["matched_string"] == "python_requires = '>=3.7'" | ||
| assert rows[0]["matched_string"] == '="python_requires = \'>=3.7\'"' |
There was a problem hiding this comment.
If we restrict the Excel-specific wrapping in version_scanner.py to only float-like strings, this test assertion can be reverted to its original clean state.
| assert rows[0]["matched_string"] == '="python_requires = \'>=3.7\'"' | |
| assert rows[0]["matched_string"] == "python_requires = '>=3.7'" |
| try: | ||
| main() | ||
| except SystemExit: | ||
| pass |
There was a problem hiding this comment.
Using pytest.raises(SystemExit) is more idiomatic and cleaner than a try-except block in pytest tests.
with pytest.raises(SystemExit):
main()| if args.stdout: | ||
| print("\n=== CSV Output ===") | ||
| import io | ||
| output = io.StringIO() | ||
| write_csv_report(output_path, all_matches, github_repo=args.github_repo, branch=args.branch) # this writes to the file, but we want stdout too | ||
| # Let's just read the file and print it | ||
| with open(output_path, 'r', encoding='utf-8') as f: | ||
| print(f.read(), end='') |
There was a problem hiding this comment.
The import io and output = io.StringIO() are unused. Additionally, write_csv_report is already called earlier in main(), so calling it again here is redundant and performs duplicate file writes. We can simply read the existing file and print it.
| if args.stdout: | |
| print("\n=== CSV Output ===") | |
| import io | |
| output = io.StringIO() | |
| write_csv_report(output_path, all_matches, github_repo=args.github_repo, branch=args.branch) # this writes to the file, but we want stdout too | |
| # Let's just read the file and print it | |
| with open(output_path, 'r', encoding='utf-8') as f: | |
| print(f.read(), end='') | |
| if args.stdout: | |
| print("\n=== CSV Output ===") | |
| with open(output_path, 'r', encoding='utf-8') as f: | |
| print(f.read(), end='') |
References
- Remove duplicate lines of code, especially duplicate assertions in tests, to keep the codebase clean and avoid redundancy.
Note
This is a work in progress.
This PR adds the version scanner to the repository and implements basic github actions as a proof of concept and a point of discussion.