check and rework wildcard pub re-export #473

echevrier · 2023-03-09T17:17:25Z

remove wildcard pub re-export if possible: re-export explicitly instead
clean the public API:
- improve primitives structure: add an extrinsics module
- update public WsRpcClient API to be similar to TungsteniteRpcClient
- dont' re-export the top level: the crate name is in the public API, as it is an important information. This should- solve the initial problem that the same type is defined in several locations.
re-order the use: use, pub use, type def
Fix issue Do not use wildcard pub re-export #430

clangenb

Thanks a lot for these fixes. I think it goes in the right direction, but I have a few remarks. :)

I see that you have removed the wildcard * re-exports in many places. I think this is a good chance, but I think only in very few cases this should be replaced by only re-exporting a subset of the definition. The questions should rather have been, do we need to re-export anything here at all, or is the wildcard re-export just obscuring that the crate structure is not so good?

So good:

no more wildcard re-exports. We should always see explicitly what is exported from a module, even if everything is re-exported.

Needs some more work:

Especially the primitives crate has a weird public api. I think this should be reworked.
Re-consider partial re-export in some cases. I think, most of the time, it makes sense to re-export everything from a module or nothing.

clangenb · 2023-03-17T13:10:53Z

node-api/src/lib.rs

-pub use storage::*;
+//re-export only "important" items: used in examples, ...
+pub use decoder::{
+	decode_as_type, decode_value_as_type, encode_as_type, encode_value_as_type, Primitive, Value,


If we do this, we should at least export the DecodeError too, be a random choice in my opinion.

However, in the case of the decoder, I believe that might be better not to re-export at all, and keep the hierarchy to have a better domain overview.

Not re-export decoder is not consistent with the other re-exports. We would have node_api::MetadataError but node_api::decoder::DecodeError.
But you're right, if we decide to re-export some part of the crate, we should not only re-export the items used in the examples.

I think I vote for not having re-exports at all in the node_api

See #473 (comment)

The decoder is needed for internal use only (by default at least). I doubt any regular user ever has to touch and use the decoder directly. And as it's pub mod, users may still access it via ::decoder::

That's exactly the opposite of EventDetails, Events and Metadata stuff, which are meant to be used by the regular users. So I vote for removing this pub reexport completely.. Except for maybe the Error types.

primitives/src/extrinsics.rs

clangenb · 2023-03-17T14:10:12Z

primitives/src/lib.rs

-pub use extrinsics::*;
-pub use pallet_traits::*;
-pub use rpc_numbers::*;
+//re-export only "important" items: items used in examples


I don't 100% agree with this comment. In my opinion, importance should rather be a matter of pub/private identifiers. From my perspective, this should be the question asked:

Does it make sense that the internal module structure is the same structure the user needs to use to import things? The answer to this question is in my opinion:

Don't re-export and keep the structure if the module represents a bigger domain.

Do re-export from modules that are only separating tiny domains, or implementation details.

There is also one other case, which is unrelated to the above question:

If you want to make a type available from another 3rd party crate, which would be needed as a dependency otherwise. However, in this case it is the same, there is no black and white answer. Sometimes it makes sense, and sometimes it doesn't.

Some consistence would be also required. See:

check and rework wildcard pub re-export #473 (comment)

check and rework wildcard pub re-export #473 (comment)

See #473 (comment)

src/rpc/ws_client/mod.rs

clangenb · 2023-03-17T14:23:58Z

src/lib.rs

+// Required wildcard re-exporting to make all items available.
+// Should not short-circuit the "privacy chain".
 pub use ac_compose_macros::*;
 pub use ac_node_api::*;
 pub use ac_primitives::*;


I think I am actually against this re-export, we have quite big domains here, and re-exporting everything here adds too many things to the top-level. I think this is also what confused my IDE. The same types were available at about 3 different locations in the path coming from here.

I do believe, that if we do not re-export these crates, we will have to add them individually over Cargo.
Yes, we have here a problem with "general" type names, like Event,Error, ... because by re-exporting these base modules, we lose the specific module name in the path.

My bad, we can use pub extern crate
But we still re-export api::* ?

Ah, sorry I misformulated this. I think we can re-export this, but only the module itself:

pub use ac_compose_macros; pub use ac_node_api; pub use ac_primitives;

hmm, maybe we can rename this:

pub use ac_compose_macros as macros; pub use ac_node_api as node_api; pub use ac_primitives as primitives;

I am unsure though.

I don't think we should rename them

clangenb · 2023-03-17T14:29:18Z

src/extrinsic/utility.rs

@@ -21,7 +21,8 @@
 use crate::{rpc::Request, Api};
 use ac_compose_macros::compose_extrinsic;
 use ac_primitives::{
-	CallIndex, ExtrinsicParams, FrameSystemConfig, SignExtrinsic, UncheckedExtrinsicV4,
+	extrinsic_params::ExtrinsicParams, extrinsics::CallIndex, FrameSystemConfig, SignExtrinsic,


This seems a bit weird to me, we have extrinsics_params::ExtrinsicParams, extrinsics::CallIndex and SignExtrinsic, which is arguably not only a matter of the re-exporting, but also how the crate is structured itself. Here it would make so much more sense to be able to do:

use ac_primitives::extrinsics::{ExtrinsicParams, CallIndex, SignExtrinsic};

To do this, we need to add an `extrinsics' module to the primitives:

extrinsics/

mod.rs (CallIndex, UncheckedExtrinsicV4)

extrinsic_params.rs

signer.rs

And we will get: use ac_primitives::extrinsics::{AssetTipExtrinsicParams, ExtrinsicParams, CallIndex, SignExtrinsic,UncheckedExtrinsicV4};

This would not be consistent with the RpcParams use ac_primitives::RpcParams. Similarly to #473 (comment)

See #473 (comment)

src/api/mod.rs

clangenb · 2023-03-17T14:35:48Z

primitives/src/lib.rs

+pub use extrinsics::UncheckedExtrinsicV4;
+pub use pallet_traits::FrameSystemConfig;
 pub use rpc_params::RpcParams;
-pub use serde_impls::*;
-pub use signer::*;
-pub use types::*;
+pub use signer::{ExtrinsicSigner, SignExtrinsic};


To me, this seems like absolutely random re-exports, I think it is better to not have any re-exports at all. This crate should export its internal types differently, this is something that makes sense to me:

// this is only for illustration for the structure. I think the crate should be // reworked to have a `extrinsics` module (in a folder). // // And **don't** do `pub use extrinsics::*` this is a prime example of a good domain // that should stay encapsulated. pub mod extrinsics { pub use extrinsics_params::*; pub use signer::*; // all the pub types that are currently in extrinsics. } // cleanly re-export rpc stuff. pub mod rpc { pub use rpc_numbers::*; pub use rpc_params::*; } // cleanly re-export other substrate types pub mod substrate { // that we have this module because we add `serde_impl` in `no_std` is an implementation detail. // The user of the api-client does not care about this. pub use serde_impl::*; // maybe we can put here the types that lived in the top-level `types` before. pub use pallet_types::*; }

This leaves three clean modules that can be referred from the outside:

pub mod extrinsics; pub mod substrate; pub mod rpc; pub mod pallet_traits;

OK. This answers my previous comment #473 (comment)
I agree that we should add modules. But I'm not sure about not re-exporting the API. We do it in the other crates. Ex:
pub use rpc_api::SubmitExtrinsic -> call from outside: use substrate_api_client::SubmitExtrinsic
but we would request for UncheckedExtrinsicV4 : use substrate_api_client::extrinsics::UncheckedExtrinsicV4

I think what's bad is that the most important info it's a primitive is no longer in the exported API, but that it's an extrinsic, it's clear, we can deduce it from the name or the context. And this also applies to the other extrinsic primitives: AssetTipExtrinsicParams, ExtrinsicParams, GenericExtrinsicParams, GenericAdditionalSigned, GenericAdditionalParams GenericSignedExtra, SignExtrinsic

Yes, I think we should not re-export it. It should probably be:

substrate_api_client::ac_primitives::extrinsics::UncheckedExtrinsicV4

See #473 (comment)

clangenb · 2023-03-17T14:59:43Z

src/rpc/ws_client/mod.rs

+pub use subscription::WsSubscriptionWrapper;
+
 use log::*;


Please order pub use to be at the end of the import statements after all regular use statements.

What/where are the guidelines? Do you mean, we should order like this:

use

pub use

pub type def
?

Yes, I think this is how we have been doing it everywhere. However, I just checked. It was never put into our coding guidelines. :)

clangenb · 2023-03-17T15:00:31Z

src/rpc/ws_client/mod.rs

@@ -18,6 +18,8 @@
 use crate::rpc::Error as RpcClientError;
 pub use ac_node_api::{events::EventDetails, StaticEvent};
 pub use client::WsRpcClient;
+pub use subscription::WsSubscriptionWrapper;


I think this is a good example, where the outside does not care if the WsSubscriptionWrapper lives inside the subscription module or not.

echevrier · 2023-03-20T10:49:26Z

The questions should rather have been, do we need to re-export anything here at all, or is the wildcard re-export just obscuring that the crate structure is not so good

I agree with it. We can't fix this wildcard re-export perfectly without thinking/reworking our structure.

echevrier · 2023-03-22T10:23:40Z

After a discussion with @clangenb we agree to :

create add module "extrinsics" in the primitives
to create a pubic API like :

- substrate_api_client::ac_primitives::UncheckedExtrinsicV4
- substrate_api_client::ac_primitives::RpcParams
- substrate_api_client::ac_primitives::BalancesConfig
- substrate_api_client::ac_primitives::AccountInfo
- substrate_api_client::ac_node_api::MetadataError
- substrate_api_client::ac_node_api::DecodeError
- substrate_api_client::ac_compose_macros::compose_extrinsic
- substrate_api_client::API
- substrate_api_client::SubmitAndWatch

haerdib · 2023-03-23T09:33:33Z

src/api/rpc_api/events.rs

@@ -12,9 +12,10 @@
 */

 use crate::{
+	ac_node_api::Phase,


Can you move that to the use ac_node_api::{EventDetails, EventRecord, Events}; ?

haerdib · 2023-03-23T10:00:23Z

src/api/rpc_api/state.rs

@@ -11,9 +11,10 @@
   limitations under the License.
 */
 use crate::{
+	ac_node_api::MetadataError,


(optional) move this outside of crate

haerdib · 2023-03-23T10:01:58Z

src/rpc/ws_client/client.rs

@@ -17,12 +17,12 @@

 use super::{subscription::WsSubscriptionWrapper, HandleMessage};
 use crate::{
+	ac_primitives::RpcParams,


(optional) move this outside of crate?

haerdib · 2023-03-23T10:10:28Z

node-api/src/lib.rs

-pub use storage::*;
+//re-export only "important" items: used in examples, ...
+pub use decoder::{
+	decode_as_type, decode_value_as_type, encode_as_type, encode_value_as_type, Primitive, Value,


The decoder is needed for internal use only (by default at least). I doubt any regular user ever has to touch and use the decoder directly. And as it's pub mod, users may still access it via ::decoder::

That's exactly the opposite of EventDetails, Events and Metadata stuff, which are meant to be used by the regular users. So I vote for removing this pub reexport completely.. Except for maybe the Error types.

- re-export only "important" items, that is items used in example - clean JsonrpseeClient API - didn't touch node-api/src/decoder as taken from parity export dependencies crate with pub extern crate instead of problematic re-export with pub use wildcard changes from review, remove invalid comments Order of uses: guideline should be: - use - pub use - type def Add extrinsics module and improve public API of ac_primitives: - Remove internal structure of extrinsics for Public API - Remove internal structure of ac_primitives for Public API: all primitives' definitions part of public ac_primitives API - Unify use path to ac_primitives types Remove unused use clean re-export decoder remove unnecessary re-export missed order use Error re-export is required Changes from review

haerdib

Cool, thanks a lot!

echevrier added F5-refactor Does not change any functionality of the code E2-breaksapi and removed F5-refactor Does not change any functionality of the code labels Mar 9, 2023

echevrier linked an issue Mar 9, 2023 that may be closed by this pull request

Do not use wildcard pub re-export #430

Closed

echevrier removed a link to an issue Mar 13, 2023

Do not use wildcard pub re-export #430

Closed

echevrier linked an issue Mar 13, 2023 that may be closed by this pull request

Do not use wildcard pub re-export #430

Closed

echevrier force-pushed the ec/pub_reexport branch from 2a09fbb to bdcf669 Compare March 13, 2023 09:36

echevrier requested review from haerdib and clangenb March 13, 2023 10:52

clangenb reviewed Mar 17, 2023

View reviewed changes

haerdib reviewed Mar 23, 2023

View reviewed changes

echevrier force-pushed the ec/pub_reexport branch from b05b09f to 26ac160 Compare March 29, 2023 13:11

echevrier requested a review from haerdib March 29, 2023 13:29

haerdib approved these changes Mar 29, 2023

View reviewed changes

echevrier merged commit 19588b1 into master Mar 29, 2023

echevrier deleted the ec/pub_reexport branch April 18, 2023 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check and rework wildcard pub re-export #473

check and rework wildcard pub re-export #473

echevrier commented Mar 9, 2023 •

edited

Loading

clangenb left a comment

clangenb Mar 17, 2023

echevrier Mar 20, 2023 •

edited

Loading

clangenb Mar 21, 2023

echevrier Mar 22, 2023

haerdib Mar 23, 2023

clangenb Mar 17, 2023

echevrier Mar 20, 2023

echevrier Mar 22, 2023

clangenb Mar 17, 2023

echevrier Mar 20, 2023

echevrier Mar 20, 2023 •

edited

Loading

clangenb Mar 21, 2023

echevrier Mar 22, 2023

clangenb Mar 17, 2023

echevrier Mar 20, 2023

echevrier Mar 22, 2023

clangenb Mar 17, 2023

echevrier Mar 20, 2023 •

edited

Loading

clangenb Mar 21, 2023

echevrier Mar 22, 2023

clangenb Mar 17, 2023

echevrier Mar 20, 2023

clangenb Mar 21, 2023

echevrier Mar 22, 2023

clangenb Mar 17, 2023

echevrier Mar 20, 2023

echevrier commented Mar 20, 2023 •

edited

Loading

echevrier commented Mar 22, 2023

haerdib Mar 23, 2023

haerdib Mar 23, 2023

haerdib Mar 23, 2023

haerdib Mar 23, 2023

haerdib left a comment

check and rework wildcard pub re-export #473

check and rework wildcard pub re-export #473

Conversation

echevrier commented Mar 9, 2023 • edited Loading

clangenb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echevrier Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echevrier Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echevrier Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echevrier commented Mar 20, 2023 • edited Loading

echevrier commented Mar 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haerdib left a comment

Choose a reason for hiding this comment

echevrier commented Mar 9, 2023 •

edited

Loading

echevrier Mar 20, 2023 •

edited

Loading

echevrier Mar 20, 2023 •

edited

Loading

echevrier Mar 20, 2023 •

edited

Loading

echevrier commented Mar 20, 2023 •

edited

Loading