Skip to content

Conversation

@ritik4ever
Copy link

@ritik4ever ritik4ever commented Jun 28, 2025

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"

- 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
@ritik4ever
Copy link
Author

Please review @cyclotruc

@cyclotruc
Copy link
Member

@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

@ix-56h
Copy link
Contributor

ix-56h commented Jun 28, 2025

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.

@ritik4ever
Copy link
Author

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

@cyclotruc
Copy link
Member

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

@ix-56h
Copy link
Contributor

ix-56h commented Jun 29, 2025

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

@NicolasIRAGNE
Copy link
Contributor

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

@cyclotruc
Copy link
Member

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

@ritik4ever ritik4ever deleted the fix/cli-malformed-argument-parsing branch July 2, 2025 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: (cli) improper parsing of malformed arguments

5 participants