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

Module structure of the Rust libraries needs cleaning up #1819

Closed
thomcc opened this issue Sep 2, 2021 · 0 comments
Closed

Module structure of the Rust libraries needs cleaning up #1819

thomcc opened this issue Sep 2, 2021 · 0 comments
Assignees

Comments

@thomcc
Copy link
Contributor

thomcc commented Sep 2, 2021

Caveat: this is subjective and quite broad.

When updating the example projects as part of #1816, I noticed a number of cases where the organization1 of different parts of the ockam rust libraries felt... suboptimal.

That is, this is just about how we group/organize things, and not about the actual APIs themselves. Specifically it feels very much like it grew organically rather than was deliberately designed.

Problems

That said, here are a couple of (mostly-) concrete problems I saw after spending a couple hours updating the examples:

  1. There are two ockam::stream modules, one is from ockam/src/stream.rs and the other is from ockam/src/protocols/stream/mod.rs, which is re-exposed by a pub use protocols::*; in the crate root (e.g. ockam/src/lib.rs). Unfortunately, the second of these rendered inaccessable2 by the first (note that protocols is not a public module).

    I suspect we have similar issues elsewhere, or could easily get them accidentally in the future. (I wish we could lint3 for this one, honestly)

  2. The subset of things that different crates re-export from eachother seems fairly arbitrary, and isn't done consistently

    • ockam_core vs ockam -- ockam re-exports a subset of ockam_core, and does not expose ockam_core.
    • ockam_vault_core vs ockam_vault -- ockam_vault re-exports a subset of ockam_value_core, but also has pub extern trait ockam_vault_core (exposing ockam_vault_core via the somewhat repetitive ockam_vault::ockam_vault_core path).
  3. We have a lot of types with colliding names, — a regex search says that we have at least 6 struct Signatures, for example.

    This isn't inherently bad — I think some of these make sense, but some of them are redundant, and others can and do cause collisions (in the examples, anyway).

    However, we should generally assume users are going to glob import the types from the traits that they need to use, and see if we can avoid that.

    Note: This one probably requires more thought...

I need to use the library a bit more, since I'm sure there's more I just didn't notice. There's also a few cases where I'm pretty sure some of the types are in a weird/seemingly-wrong crate, but I need to look more to say for certain.

Rough / Opinionated notes

Here are some rough thoughts that reflect some of my opinions on how we should maybe do things. Most of this is not specific to ockam at all.

  • Stuff like ockam_foo/ockam_foo_core should probably either:

    • be analagous to std and core from the stdlib (where std contains the entireity of core directly).
    • have ockam_foo_core available as ockam_foo::core.
  • We should use way less use globbing. I don't think there we want to stop doing it all-together, but we use it much more than we need to.

  • Most of our small crates are fine, but for larger (in terms of API complexity) and front-facing crates, it needs more careful organization, and to export way less things from the root.

    • For example the ockam crate has 148 public exports from its root, which is far too many. Many of these should exposed in modules.
  • Crates that want to provide easy access to commonly used stuff for convenience should put it in a pub mod prelude.

  • In cases where we're exporting a bunch of types from another ockam crate (and/or the other ockam crate is used all over the public API of this crate), we should probably just reexport the crate.

    • Probably with a rename to avoid duplication — ockam::foo rather than ockam::ockam_foo.
    • Note that this is a lot less of a good idea for crates we don't control, hence me specifying "ockam crate"
  • (Note: some crate families to look at for design inspiration are: std/core/alloc (e.g. the stdlib), tokio, tracing, bevy...)

This is all very abstract though, so I need to spend some time making more concrete thoughts about the changes.


1: I'm using "organization" here to refer to something like "the module and crate structure, as well as which types and modules are exported (or re-exported) where, and under what names". (It's not a very concrete definition, but that should get you most of the way there to what I mean).

2: We had code in the example projects that used it, at the very least.

3: This is rust-lang/rust-clippy#7432. If we care a lot about this, I think it wouldn't be hard to do (it would be very similar to the other glob import lints).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants