Page MenuHomePhabricator

rhg: replace command structs with functions
ClosedPublic

Authored by SimonSapin on Mon, Feb 8, 5:43 PM.

Details

Summary

The Command trait was not used in any generic context,
and the struct where nothing more than holders for values parsed from CLI
arguments to be available to a run method.

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

SimonSapin created this revision.Mon, Feb 8, 5:43 PM
Alphare accepted this revision.Tue, Feb 9, 3:53 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/rhg/src/commands/cat.rs
22

I just noticed that this uses values_of instead of values_of_os, so non-UTF8 filenames will cause a panic. We should address that in another patch.

Now that I think about it, we should also have the patch with catch_unwind we've been talking about for a while, to fallback to Python in case we get a panic.

SimonSapin added inline comments.Tue, Feb 9, 4:10 AM
rust/rhg/src/commands/cat.rs
22

I’m not thrilled about silently falling back on panics instead of only when we opt to with HgError::unimplemented. Panics indicate that should be fixed and so need to be noticed.

Alphare added inline comments.Tue, Feb 9, 4:12 AM
rust/rhg/src/commands/cat.rs
22

You're probably right. The catch_unwind patch would probably be for later to add some user-facing message like what hg does with a link to the bug tracker, etc.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.