comit-network / comit-rs

Reference implementation of COMIT, an open protocol facilitating trustless cross-blockchain applications.
GNU General Public License v3.0
191 stars 33 forks source link

Prelude module for the `comit` library #2884

Closed thomaseizinger closed 4 years ago

thomaseizinger commented 4 years ago

Moving @D4nte idea to a separate thread so we don't clutter the discussion in the PR:

Original comment: https://github.com/coblox/nectar/pull/19#pullrequestreview-433838119

Not that related here but it would be interesting to have comit export a prelude module that contains all the necessary stuff so you can do:

pub use comit::prelude::*;

instead of

  pub use comit::{
        actions::bitcoin::{BroadcastSignedTransaction, SendToAddress},
        btsieve::{BlockByHash, LatestBlock},
        hbit::*,
        htlc_location, transaction, Secret,
    };
thomaseizinger commented 4 years ago

My two cents on this are:

We should clearly define what the purpose of the prelude module is and why this cannot be achieved by re-arranging the actual modules themselves, thereby making use comit::* basically the prelude.

Two limitations of doing use comit::* come to my mind:

Generally, I feel like it is too early to take these kind of steps. We have little experience on what even should be in the comit library and a prelude module is really an optimization once certain usage patterns repeat themselves. I feel like there is little return on an investment into this topic now.

D4nte commented 4 years ago

My two cents on this are:

We should clearly define what the purpose of the prelude module is and why this cannot be achieved by re-arranging the actual modules themselves, thereby making use comit::* basically the prelude.

Well, comit::* and comit::prelude::* are not the same. I would prefer to suggest using prelude because it is about bundling everything that a user might need. They can use comit::foo::Foo to export specific type. I would not want to re-export everything from the crate root. Please note that this idea comes from the video you recently shared who mentioned the lengthy import sections hidden by the IDE.

Finally, prelude is a practice I have seen in Rust crates. using foo::* is not.

Two limitations of doing use comit::* come to my mind:

* Sometimes you need traits to be in scope, just so you can call their functions. In those cases you don't care about their name and the prelude could contain something like: `use btsieve::BlockByHash as _;`

True and this looks like a good reason to use prelude.

* Some names are unambiguous throughout the whole library, yet still contained in a submodule. For example, `BroadcastSignedTransaction`. Yet, you would still probably want to keep them organized in the submodule.

To me, those two points say comit::prelude::* has less cons than comit::*.

Generally, I feel like it is too early to take these kind of steps. We have little experience on what even should be in the comit library and a prelude module is really an optimization once certain usage patterns repeat themselves. I feel like there is little return on an investment into this topic now.

I would tend to disagreed as you could just add everything that nectar uses today and when a second app is added, then review and upgrade.

thomaseizinger commented 4 years ago

To me, those two points say comit::prelude::* has less cons than comit::*.

Yes, these two points are things you couldn't do with use comit::* as nicely.

I would tend to disagreed as you could just add everything that nectar uses today and when a second app is added, then review and upgrade.

What I think is worthwhile avoiding is a back and forth between:

If you want to experiment with a comit prelude, you can do that just as easily in nectar itself, by creating your own module that re-exports things from comit. That should give you the same benefits with less churn when iterating and can be upstreamed once it stabilizes :)

github-actions[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.