This is an archive of the discontinued Mercurial Phabricator instance.

errors: introduce InputError and use it from commands and cmdutil
ClosedPublic

Authored by martinvonz on Oct 7 2020, 2:10 AM.

Details

Summary

This patch introduces a InputError class and replaces many uses of
error.Abort by it in commands and cmdutil. This is a part of
https://www.mercurial-scm.org/wiki/ErrorCategoriesPlan. There will
later be a different class for state errors (to raise e.g. when
there's an unfinished operation). It's not always clear when one
should report an input error and when it should be a state error. We
can always adjust later if I got something wrong in this patch (but
feel free to point out any you notice now).

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.Oct 7 2020, 2:10 AM
martinvonz updated this revision to Diff 23073.Oct 7 2020, 2:17 AM
pulkit added a subscriber: pulkit.Oct 7 2020, 7:53 AM

The plan page mentions InputError instead of UserError which I think is more fine-grained and better suited in some cases. Reading the relevant email thread titled Proposal for cleaning up error reporting, it seems that we have not finalized these names.

The plan page mentions InputError instead of UserError which I think is more fine-grained and better suited in some cases.

I felt like InputError made it sounds like it was specifically for errors in input when hg was asking for input, not including inputs given on the command line. I'm fine with using the proposed InputError, though. I'll wait a bit to hear other people's opinions first.

Reading the relevant email thread titled Proposal for cleaning up error reporting, it seems that we have not finalized these names.

Sorry, I clearly should have reviewed that thread first. I thought the thread ended with Augie's message about forcing opinions from non-Googlers by sending a patch. I also remember saying that I was skeptical about changing error statuses. Apparently both those memories were incorrect :) However, the thread I reviewed just now doesn't seem to talk about the names at all. Were there several threads?

martinvonz retitled this revision from errors: introduce UserError and use it from commands and cmdutil to errors: introduce InputError and use it from commands and cmdutil.Oct 12 2020, 2:29 PM
martinvonz edited the summary of this revision. (Show Details)
martinvonz updated this revision to Diff 23177.

I've renamed the exception to InputError to unblock this. Let me know if there was anything else blocking. I wasn't sure if you wanted to see more discussion about the name on thread before we decide.

I've renamed the exception to InputError to unblock this. Let me know if there was anything else blocking. I wasn't sure if you wanted to see more discussion about the name on thread before we decide.

Thank you. I am thinking to get back to this (queue this maybe) once we cut the upcoming RC (after 3 days). I agree that there are no BC changes but doing this will make things easier as then we can try to have all these changes in one release. Doesn't yield much but sounds like a good idea to me. Let me know if you prefer it to be queued before.

I've renamed the exception to InputError to unblock this. Let me know if there was anything else blocking. I wasn't sure if you wanted to see more discussion about the name on thread before we decide.

Thank you. I am thinking to get back to this (queue this maybe) once we cut the upcoming RC (after 3 days). I agree that there are no BC changes but doing this will make things easier as then we can try to have all these changes in one release. Doesn't yield much but sounds like a good idea to me. Let me know if you prefer it to be queued before.

That sounds reasonable. There's no rush with this from our side. Thanks.

martinvonz planned changes to this revision.Oct 22 2020, 11:37 AM

Actually, let me insert a patch before this one that adds the config that gives us more detailed exit codes. That way we can see the impact on tests in this patch.

martinvonz updated this revision to Diff 23370.Oct 29 2020, 4:39 PM
martinvonz updated this revision to Diff 23417.Nov 3 2020, 11:45 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.