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

Make otp-cache a binary target of nitrocli #169

Closed
wants to merge 2 commits into from

Conversation

robinkrahl
Copy link
Collaborator

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
Copy link
Owner

d-e-s-o commented Apr 25, 2021

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
Copy link
Collaborator Author

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
Copy link
Owner

d-e-s-o commented Apr 25, 2021

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
Copy link
Collaborator Author

robinkrahl commented Apr 25, 2021

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.

This is intended as an example for discussion in d-e-s-o#166.
@d-e-s-o
Copy link
Owner

d-e-s-o commented Apr 30, 2021

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
Copy link
Collaborator Author

robinkrahl commented May 2, 2021 via email

@robinkrahl robinkrahl marked this pull request as ready for review May 2, 2021 08:56
@d-e-s-o
Copy link
Owner

d-e-s-o commented May 9, 2021

I merged the change. Thanks Robin!

@d-e-s-o d-e-s-o closed this May 9, 2021
@d-e-s-o d-e-s-o mentioned this pull request May 9, 2021
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.

None yet

2 participants