fix(cli): Add Click validation for malformed long options
Fix: CLI Improper Parsing of Malformed Arguments close #319
Problem
When users typed -include-gitignored, Click incorrectly parsed it as -i include-gitignored instead of showing a helpful error message.
Solution
Added Click parameter validation callbacks that:
- Detect malformed long options in pattern values
- Handle Click's character consumption (reconstructs
include-gitignoredfromnclude-gitignored) - Show helpful error messages with correct usage examples
- Preserve all existing valid usage patterns
Testing
Before (broken):
$ gitingest -include-gitignored repo
# Silently parsed as -i "include-gitignored"
Please review @cyclotruc
@ritik4ever Thanks for your contribution! Did we actually need that much validation code? I'm a bit surprised that there's not a more idiomatic way to achieve this with Click
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 :
- Rename abbreviation ->
-ipfor--include-pattern - Rename arguments ->
--include-patternto--pattern
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. I'm a bit surprised.
@ritik4ever Thanks for your contribution! Did we actually need that much validation code? I'm a bit surprised that there's not a more idiomatic way to achieve this with Click
@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) This would remove the ambiguity that led to the original issue.
Option 2: Rename the arguments
--include-pattern → --pattern
--exclude-pattern → --exclude This keeps things short, clear, and avoids any overlap entirely.
Would you prefer me to:
- Implement Option 1 (change abbreviations to
-ip/-ep) - Implement Option 2 (rename to
--pattern/--exclude)
@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?
@ritik4ever I'm not convinced that renaming/changing the arguments is the way to go either
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.
I still believe there should be a more idiomatic way to properly parse CLI arguments
After a bit more digging, I couldn't find a native solution to this problem. We must bear in mind that this is a user input error and not a bug. We should be able to add a warning when using the include pattern option, but this is quite odd.
maybe by switching to a different CLI framework? if anyone has experience with this?
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 -i abbreviation is certainly the best way to solve the problem, even if it means introducing a breaking change.
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 -xyz as -x -y -z so we'll not be changing this
thanks anyway for the effort @ritik4ever, much appreciated, sorry for the last minute change of mind