Fix documentation duplication, CLI argument parsing, and workflow artifacts #198
Conversation
Co-authored-by: 73junito <86015877+73junito@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses review feedback from PR #180 by fixing three distinct issues: removing 99 lines of duplicate documentation, adding CLI argument parsing for quality control parameters, and removing invalid workflow flags that don't exist in the script.
Changes:
- Added CLI argument parsing for
--min-tokensand--min-charsin the orchestrate script - Removed duplicate documentation content and redundant command example from AI toolkit guidance
- Removed non-existent
--outputand--log-ttftflags from the workflow and eliminated artifact upload for files that aren't generated
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| scripts/orchestrate_content.py | Adds CLI parsing for --min-tokens and --min-chars arguments, updates usage message, and improves exception handling consistency |
| docs/ai/ai-toolkit-guidance.md | Removes duplicate documentation sections and redundant command example |
| .github/workflows/orchestrate-test.yml | Removes invalid CLI flags and artifact upload step for non-existent output files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except Exception: | ||
| concurrency = 4 | ||
| pass |
There was a problem hiding this comment.
The catch-all except Exception: pass blocks for CLI argument parsing are overly broad and could silently swallow unexpected errors. Consider removing these blocks and only catching (ValueError, IndexError), which already handles the expected error cases when the flag is missing or the value cannot be parsed.
| except Exception: | ||
| pass |
There was a problem hiding this comment.
The catch-all except Exception: pass blocks for CLI argument parsing are overly broad and could silently swallow unexpected errors. Consider removing these blocks and only catching (ValueError, IndexError), which already handles the expected error cases when the flag is missing or the value cannot be parsed.
| except Exception: | ||
| pass |
There was a problem hiding this comment.
The catch-all except Exception: pass blocks for CLI argument parsing are overly broad and could silently swallow unexpected errors. Consider removing these blocks and only catching (ValueError, IndexError), which already handles the expected error cases when the flag is missing or the value cannot be parsed.
| # Parse --min-tokens | ||
| min_tokens = DEFAULT_MIN_TOKENS | ||
| try: | ||
| idx = sys.argv.index("--min-tokens") | ||
| min_tokens = int(sys.argv[idx + 1]) | ||
| except (ValueError, IndexError): | ||
| pass | ||
| except Exception: | ||
| pass | ||
|
|
||
| # Parse --min-chars | ||
| min_chars = DEFAULT_MIN_CHARS | ||
| try: | ||
| idx = sys.argv.index("--min-chars") | ||
| min_chars = int(sys.argv[idx + 1]) | ||
| except (ValueError, IndexError): | ||
| pass | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
The new CLI argument parsing for --min-tokens and --min-chars lacks test coverage. While the underlying orchestrate() function is tested with these parameters, the CLI parsing logic in main() that extracts these values from sys.argv is not tested. Consider adding tests that verify the arguments are correctly parsed from the command line and passed to the orchestrate function.
Summary
--min-tokensand--min-charsin code but couldn't parse them from CLI; workflow referenced non-existent output files.Changes
--min-tokensand--min-charsargument parsing inmain(). Updated usage message.--outputand--log-ttftflags; removed artifact upload step for non-existent files.Testing
test_orchestrate_content.pypass.Checklist
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.