Page MenuHomePhabricator

errors: raise InputError on early parse error in dispatch
ClosedPublic

Authored by martinvonz on Nov 24 2020, 1:05 PM.

Details

Summary

I didn't think this would have any effect on the tests, but it does
because the catching in scmutil.callcatch() still happens. That's
because dispatch passes in the function that includes the parsing as
an argument to that function.

I initially used ConfigError here but Matt Harbison convinced me to
use InputError. I think that makes sense since error is not in a
config file.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.Nov 24 2020, 1:05 PM

If this is purely from --config instead of a file and you're collecting metrics, would you want this to be an InputError? (I'm happy to queue this as-is, I just wanted to double check)

If this is purely from --config instead of a file and you're collecting metrics, would you want this to be an InputError? (I'm happy to queue this as-is, I just wanted to double check)

I intentionally used ConfigError here, but it's a question and you may be right that InputError is more correct. The user should not try to fix it by modifying config files, so you're probably right. I'll change it.

pulkit requested changes to this revision.Dec 4 2020, 1:50 PM
pulkit added a subscriber: pulkit.

Seems like there are already changes planned on this one.

This revision now requires changes to proceed.Dec 4 2020, 1:50 PM
martinvonz retitled this revision from errors: raise ConfigError on early parse error in dispatch to errors: raise InputError on early parse error in dispatch.Dec 14 2020, 4:31 PM
martinvonz edited the summary of this revision. (Show Details)
martinvonz updated this revision to Diff 24269.

Seems like there are already changes planned on this one.

Yes. I think that should now be done.

mharbison72 accepted this revision.Dec 15 2020, 10:59 PM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.