gitingest icon indicating copy to clipboard operation
gitingest copied to clipboard

fix(cli): Add Click validation for malformed long options

Open ritik4ever opened this issue 6 months ago • 6 comments

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-gitignored from nclude-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"

ritik4ever avatar Jun 28 '25 08:06 ritik4ever

Please review @cyclotruc

ritik4ever avatar Jun 28 '25 08:06 ritik4ever

@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 avatar Jun 28 '25 16:06 cyclotruc

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 -> -ip for --include-pattern
  • Rename arguments -> --include-pattern to --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.

ix-56h avatar Jun 28 '25 18:06 ix-56h

@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:

  1. Implement Option 1 (change abbreviations to -ip/-ep)
  2. Implement Option 2 (rename to --pattern/--exclude)

ritik4ever avatar Jun 29 '25 02:06 ritik4ever

@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?

cyclotruc avatar Jun 29 '25 13:06 cyclotruc

@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.

ix-56h avatar Jun 29 '25 16:06 ix-56h

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

NicolasIRAGNE avatar Jul 01 '25 21:07 NicolasIRAGNE

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

cyclotruc avatar Jul 01 '25 21:07 cyclotruc