Skip to content
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

Give a more succinct error message when user specifies nonexistent files #637

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cincodenada
Copy link

@cincodenada cincodenada commented Jan 29, 2025

Currently specifying nonexistent files to options gives a whole Python backtrace, with the filename output in b'filename' bytestring notation which is even more confusing.

There are plenty of approaches here, this just catches any FileNotFoundErrors and raises a more succinct error message about it, which doesn't tell you which is the offending argument, but that should be apparent enough, and in any case, this seems to me like strictly an improvement over the backtrace that currently happens.

Otherwise we get a whole backtrace which is alarming

Signed-off-by: Ell Bradshaw <[email protected]>
Copy link
Owner

@newren newren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Thanks for sending this in. The general idea seems good, but this particular placement feels a bit to coarse to me; I'd rather have the checks around get_replace_text() and the usage of args.strip_blobs_with_ids.

Also, the commit message mostly looks good, but to follow the git style the first line should be "area: summary", e.g. "filter-repo: handle nonexistent files more nicely" (also notice the lack of capitalization after the colon). It may also be worth adding in the text after the first line that this is specifically about argument parsing, but the main thing is just the "area: summary" bit.

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.

2 participants