d-e-s-o / nitrocli

A command line tool for interacting with Nitrokey devices.
30 stars 10 forks source link

Make otp-cache a binary target of nitrocli #169

Closed robinkrahl closed 3 years ago

robinkrahl commented 3 years ago

This is intended as an example for discussion in #166. Unforunately, it is not possible to directly include modules from the main binary, so we use

    #[path = "../ext.rs"]
    mod ext;

as a workaround to share code between the extensions.

d-e-s-o commented 3 years ago

Thanks for making this change! Looks good to me. The only thing that I think we should change is to keep all things extensions below ext/. This makes the rather strict separation from the main program more clear. I'd want to reserve src/ strictly for nitrocli proper (yes, it's a bit of a misnomer then but let's accept that for now; I don't believe that we are alone in this boat).

If that makes sharing code between extensions and nitrocli harder then I also think that's a good thing. We should be very deliberate about that.

robinkrahl commented 3 years ago

The only thing that I think we should change is to keep all things extensions below ext/.

What about the ext module? Should it be ext/mod.rs or src/ext.rs?

d-e-s-o commented 3 years ago

The only thing that I think we should change is to keep all things extensions below ext/.

What about the ext module? Should it be ext/mod.rs or src/ext.rs?

I'd just go with <nitrocli-root>/ext/ext.rs. If we decide to go multi-file we'd make that a directory <nitrocli-root>/ext/ext/{mod,impl}.rs. I suppose that should "just work" then. We can also do that right away if you feel that's a distinct possibility.

robinkrahl commented 3 years ago

I'd just go with <nitrocli-root>/ext/ext.rs.

Okay! Though I was just happy to get rid of the ext/ext duplication in the path. ;) On the positive site, this allows us to get rid of the #[path = "..."] workaround.

d-e-s-o commented 3 years ago

Shall we merge this pull request, Robin? I also think we can remove Context::text for now if it's not actively used (but I can take care of that).

robinkrahl commented 3 years ago

If we want to use this approach, sure. I’ve removed the Nitrocli::text method, but we’ll probably have to add #[allow(unused)] in the future anyway because not all extensions will use all parts of the interface.

d-e-s-o commented 3 years ago

I merged the change. Thanks Robin!