-
Notifications
You must be signed in to change notification settings - Fork 1k
fix(cli): Add Click validation for malformed long options #336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(cli): Add Click validation for malformed long options #336
Conversation
- Detect single-dash long options like -include-gitignored - Provide helpful error messages suggesting correct format - Add validation callbacks to include/exclude pattern options - Handle Click's character consumption with smart reconstruction - Maintain backward compatibility for all valid usage patterns Key changes: - Created src/gitingest/utils/click_validation_utils.py with validation logic - Modified src/gitingest/cli.py to add validation imports and callbacks - Added tests/test_cli_validation.py with test coverage Fixes coderamp-labs#319
|
Please review @cyclotruc |
|
@ritik4ever Thanks for your contribution! |
|
I don't think this is something we should validate, but rather a question of how we plan to handle cli arguments. Writing validation for this kind of edge case can lead to a lot of code in the future. Conflicting arguments are a pain to deal with, but for this specific problem, the way to proceed should be to remove abbreviations for optional arguments, and especially for similar names. Keeping things simple is the way to go, I think. Anyway, if you really want abbreviation, maybe one of these solutions would be better :
Finally, I did a little digging and it seems that click doesn't provide any way to make this kind of validation explicit for this specific case. |
@cyclotruc @ix-56h thanks a lot for the feedback, i agree , simplicity and addressing the root cause directly is the way to go. Here are the two options I’m considering based on your input: Option 1: Adjust the abbreviations Change -i to -ip (for --include-pattern) Change -e to -ep (for --exclude-pattern) Option 2: Rename the arguments --include-pattern → --pattern --exclude-pattern → --exclude Would you prefer me to:
|
|
@ritik4ever I'm not convinced that renaming/changing the arguments is the way to go either I still believe there should be a more idiomatic way to properly parse CLI arguments, using Click or maybe by switching to a different CLI framework? if anyone has experience with this? |
This is certainly a solution, currently this behavior is intended by the linux argument style and cannot be avoided unless you move away from the holy unix and click style by providing enough validations.
After a bit more digging, I couldn't find a native solution to this problem.
Click remains a standard, but we can check Typer by Tiangolo, even if the way it works may seem a bit odd it remains a valid solution. From my point of view, deleting the |
|
Intended behavior, might be confusing to people who are not used to CLI but this is a common way to parse arguments and I think trying to put safeguards around that will do more harm than good in the end |
|
Yeah after further inspection it is intented by the author of click to support |
Fix: CLI Improper Parsing of Malformed Arguments close #319
Problem
When users typed
-include-gitignored, Click incorrectly parsed it as-i include-gitignoredinstead of showing a helpful error message.Solution
Added Click parameter validation callbacks that:
include-gitignoredfromnclude-gitignored)Testing
Before (broken):
$ gitingest -include-gitignored repo # Silently parsed as -i "include-gitignored"