Check docs and run clippy on PRs (paritytech#326)

* Check docs, clippy, test run

* test parallel CI adapted from other package; is it faster?

* Remember to download substrate

* Nightly for cargo fmt

* Standardise CI names

* fix clippy complaints

* Ensure docs are valid and export publicly accessible types

* all-targets clippy, and fix additional lint errors

* newline in ci file

.github/workflows/rust.yml

@@ -2,39 +2,137 @@ name: Rust
 
 on:
   push:
-    branches: [ master ]
+    # Run jobs when commits are pushed to
+    # master or release-like branches:
+    branches:
+      - master
   pull_request:
-    branches: [ master ]
+    # Run jobs for any external PR that wants
+    # to merge to master, too:
+    branches:
+      - master
+
+env:
+  CARGO_TERM_COLOR: always
 
 jobs:
   build:
+    name: Cargo check
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout sources
+        uses: actions/checkout@v2
+
+      - name: Install Rust stable toolchain
+        uses: actions-rs/toolchain@v1
+        with:
+          profile: minimal
+          toolchain: stable
+          override: true
+
+      - name: Rust Cache
+        uses: Swatinem/[email protected]
+
+      - name: Build
+        uses: actions-rs/[email protected]
+        with:
+          command: check
+          args: --all-targets --all-features --workspace
+
+  fmt:
+    name: Cargo fmt
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout sources
+        uses: actions/checkout@v2
+
+      - name: Install Rust nightly toolchain
+        uses: actions-rs/toolchain@v1
+        with:
+            profile: minimal
+            toolchain: nightly
+            override: true
+            components: rustfmt
+
+      - name: Rust Cache
+        uses: Swatinem/[email protected]
 
+      - name: Cargo fmt
+        uses: actions-rs/[email protected]
+        with:
+          command: fmt
+          args: --all -- --check
+
+  docs:
+    name: Check documentation
     runs-on: ubuntu-latest
+    steps:
+      - name: Checkout sources
+        uses: actions/checkout@v2
+
+      - name: Install Rust stable toolchain
+        uses: actions-rs/toolchain@v1
+        with:
+          profile: minimal
+          toolchain: stable
+          override: true
+
+      - name: Rust Cache
+        uses: Swatinem/[email protected]
+
+      - name: Check internal documentation links
+        run: RUSTDOCFLAGS="--deny broken_intra_doc_links" cargo doc --verbose --workspace --no-deps --document-private-items
 
+  tests:
+    name: Cargo test
+    runs-on: ubuntu-latest
     steps:
-    - uses: actions/checkout@v2
+      - name: Checkout sources
+        uses: actions/checkout@v2
+
+      - name: Download Substrate
+        run: |
+            curl "https://releases.parity.io/substrate/x86_64-debian:stretch/latest/substrate/substrate" --output substrate --location
+            chmod +x ./substrate
+            mkdir -p ~/.local/bin
+            mv substrate ~/.local/bin
 
-    - name: setup
-      uses: actions-rs/toolchain@v1
-      with:
+      - name: Install Rust stable toolchain
+        uses: actions-rs/toolchain@v1
+        with:
           profile: minimal
-          toolchain: nightly
+          toolchain: stable
           override: true
-          components: rustfmt
-          target: wasm32-unknown-unknown
 
-    - name: download-substrate
-      run: |
-          curl "https://releases.parity.io/substrate/x86_64-debian:stretch/latest/substrate/substrate" --output substrate --location
-          chmod +x ./substrate
-          mkdir -p ~/.local/bin
-          mv substrate ~/.local/bin
+      - name: Rust Cache
+        uses: Swatinem/[email protected]
+
+      - name: Cargo test
+        uses: actions-rs/[email protected]
+        with:
+          command: test
+          args: --all-targets --workspace
+
+  clippy:
+    name: Cargo clippy
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout sources
+        uses: actions/checkout@v2
 
-    - name: fmt
-      run: cargo fmt --all -- --check
+      - name: Install Rust stable toolchain
+        uses: actions-rs/toolchain@v1
+        with:
+            profile: minimal
+            toolchain: stable
+            components: clippy
+            override: true
 
-    - name: build
-      run: cargo build --workspace --verbose
+      - name: Rust Cache
+        uses: Swatinem/[email protected]
 
-    - name: test
-      run: cargo test --workspace --verbose
+      - name: Run clippy
+        uses: actions-rs/cargo@v1
+        with:
+          command: clippy
+          args: --all-targets -- -D warnings

.gitignore

@@ -1,4 +1,5 @@
 /target
 **/*.rs.bk
+**/.DS_Store
 Cargo.lock
-cargo-timing*
+cargo-timing*

codegen/src/api/storage.rs

@@ -44,7 +44,7 @@ pub fn generate_storage(
     let (storage_structs, storage_fns): (Vec<_>, Vec<_>) = storage
         .entries
         .iter()
-        .map(|entry| generate_storage_entry_fns(&type_gen, &pallet, entry))
+        .map(|entry| generate_storage_entry_fns(type_gen, pallet, entry))
         .unzip();
 
     quote! {

codegen/src/ir.rs

@@ -88,12 +88,9 @@ impl From<syn::Item> for Item {
                     let meta = attr.parse_meta().unwrap_or_else(|e| {
                         abort!(attr.span(), "Error parsing attribute: {}", e)
                     });
-                    let substitute_type_args =
-                        <attrs::Subxt as darling::FromMeta>::from_meta(&meta)
-                            .unwrap_or_else(|e| {
-                                abort!(attr.span(), "Error parsing attribute meta: {}", e)
-                            });
-                    substitute_type_args
+                    <attrs::Subxt as darling::FromMeta>::from_meta(&meta).unwrap_or_else(
+                        |e| abort!(attr.span(), "Error parsing attribute meta: {}", e),
+                    )
                 })
                 .collect::<Vec<_>>();
             if substitute_attrs.len() > 1 {
@@ -102,11 +99,11 @@ impl From<syn::Item> for Item {
                     "Duplicate `substitute_type` attributes"
                 )
             }
-            if let Some(attr) = substitute_attrs.iter().next() {
+            if let Some(attr) = substitute_attrs.get(0) {
                 let use_path = &use_.tree;
                 let substitute_with: syn::TypePath = syn::parse_quote!( #use_path );
                 let type_substitute = SubxtItem::TypeSubstitute {
-                    generated_type_path: attr.substitute_type().to_string(),
+                    generated_type_path: attr.substitute_type(),
                     substitute_with,
                 };
                 Self::Subxt(type_substitute)

src/events.rs

@@ -100,7 +100,7 @@ where
             let mut event_data = Vec::<u8>::new();
             let mut event_errors = Vec::<RuntimeError>::new();
             let result = self.decode_raw_event(
-                &event_metadata,
+                event_metadata,
                 input,
                 &mut event_data,
                 &mut event_errors,
@@ -204,9 +204,13 @@ where
             TypeDef::Variant(variant) => {
                 let variant_index = u8::decode(input)?;
                 variant_index.encode_to(output);
-                let variant = variant.variants().get(variant_index as usize).ok_or(
-                    Error::Other(format!("Variant {} not found", variant_index)),
-                )?;
+                let variant =
+                    variant
+                        .variants()
+                        .get(variant_index as usize)
+                        .ok_or_else(|| {
+                            Error::Other(format!("Variant {} not found", variant_index))
+                        })?;
                 for field in variant.fields() {
                     self.decode_type(field.ty().id(), input, output)?;
                 }
@@ -299,15 +303,21 @@ where
                     TypeDef::Composite(composite) => {
                         match composite.fields() {
                             [field] => {
-                                let field_ty =
-                                    self.metadata.resolve_type(field.ty().id()).ok_or(
-                                        MetadataError::TypeNotFound(field.ty().id()),
-                                    )?;
+                                let field_ty = self
+                                    .metadata
+                                    .resolve_type(field.ty().id())
+                                    .ok_or_else(|| {
+                                        MetadataError::TypeNotFound(field.ty().id())
+                                    })?;
                                 if let TypeDef::Primitive(primitive) = field_ty.type_def()
                                 {
                                     decode_compact_primitive(primitive)
                                 } else {
-                                    Err(EventsDecodingError::InvalidCompactType("Composite type must have a single primitive field".into()).into())
+                                    Err(EventsDecodingError::InvalidCompactType(
+                                    "Composite type must have a single primitive field"
+                                        .into(),
+                                )
+                                .into())
                                 }
                             }
                             _ => {

src/lib.rs

@@ -96,7 +96,11 @@ pub use crate::{
         Signer,
         UncheckedExtrinsic,
     },
-    metadata::Metadata,
+    metadata::{
+        Metadata,
+        MetadataError,
+        PalletMetadata,
+    },
     rpc::{
         BlockNumber,
         ExtrinsicSuccess,
@@ -167,10 +171,8 @@ pub enum Phase {
 
 /// A wrapper for any type `T` which implement encode/decode in a way compatible with `Vec<u8>`.
 ///
-/// This type is similar to [`WrapperOpaque`], but it differs in the way it stores the type `T`.
-/// While [`WrapperOpaque`] stores the decoded type, the [`WrapperKeepOpaque`] stores the type only
-/// in its opaque format, aka as a `Vec<u8>`. To access the real type `T` [`Self::try_decode`] needs
-/// to be used.
+/// [`WrapperKeepOpaque`] stores the type only in its opaque format, aka as a `Vec<u8>`. To
+/// access the real type `T` [`Self::try_decode`] needs to be used.
 #[derive(Debug, Eq, PartialEq, Default, Clone, Decode, Encode)]
 pub struct WrapperKeepOpaque<T> {
     data: Vec<u8>,
@@ -182,7 +184,7 @@ impl<T: Decode> WrapperKeepOpaque<T> {
     ///
     /// Returns `None` if the decoding failed.
     pub fn try_decode(&self) -> Option<T> {
-        T::decode_all(&mut &self.data[..]).ok()
+        T::decode_all(&self.data[..]).ok()
     }
 
     /// Returns the length of the encoded `T`.

src/metadata.rs

@@ -73,6 +73,7 @@ pub enum MetadataError {
     /// Constant is not in metadata.
     #[error("Constant {0} not found")]
     ConstantNotFound(&'static str),
+    /// Type is not in metadata.
     #[error("Type {0} missing from type registry")]
     TypeNotFound(u32),
 }
@@ -91,7 +92,7 @@ impl Metadata {
     pub fn pallet(&self, name: &'static str) -> Result<&PalletMetadata, MetadataError> {
         self.pallets
             .get(name)
-            .ok_or(MetadataError::PalletNotFound(name.to_string()))
+            .ok_or_else(|| MetadataError::PalletNotFound(name.to_string()))
     }
 
     /// Returns the metadata for the event at the given pallet and event indices.
@@ -131,6 +132,7 @@ impl Metadata {
     }
 }
 
+/// Metadata for a specific pallet.
 #[derive(Clone, Debug)]
 pub struct PalletMetadata {
     index: u8,
@@ -141,6 +143,7 @@ pub struct PalletMetadata {
 }
 
 impl PalletMetadata {
+    /// Encode a call based on this pallet metadata.
     pub fn encode_call<C>(&self, call: &C) -> Result<Encoded, MetadataError>
     where
         C: Call,
@@ -154,6 +157,7 @@ impl PalletMetadata {
         Ok(Encoded(bytes))
     }
 
+    /// Return [`StorageEntryMetadata`] given some storage key.
     pub fn storage(
         &self,
         key: &'static str,
@@ -163,7 +167,7 @@ impl PalletMetadata {
             .ok_or(MetadataError::StorageNotFound(key))
     }
 
-    /// Get a constant's metadata by name
+    /// Get a constant's metadata by name.
     pub fn constant(
         &self,
         key: &'static str,
@@ -239,11 +243,11 @@ impl TryFrom<RuntimeMetadataPrefixed> for Metadata {
 
     fn try_from(metadata: RuntimeMetadataPrefixed) -> Result<Self, Self::Error> {
         if metadata.0 != META_RESERVED {
-            return Err(InvalidMetadataError::InvalidPrefix.into())
+            return Err(InvalidMetadataError::InvalidPrefix)
         }
         let metadata = match metadata.1 {
             RuntimeMetadata::V14(meta) => meta,
-            _ => return Err(InvalidMetadataError::InvalidVersion.into()),
+            _ => return Err(InvalidMetadataError::InvalidVersion),
         };
 
         let get_type_def_variant = |type_id: u32| {

src/rpc.rs

@@ -616,10 +616,10 @@ impl<T: Config> Rpc<T> {
         Err(RpcError::Custom("RPC subscription dropped".into()).into())
     }
 
-    async fn process_block<'a>(
+    async fn process_block(
         &self,
         events_sub: EventStorageSubscription<T>,
-        decoder: &'a EventsDecoder<T>,
+        decoder: &EventsDecoder<T>,
         block_hash: T::Hash,
         ext_hash: T::Hash,
     ) -> Result<ExtrinsicSuccess<T>, Error> {

src/subscription.rs

@@ -391,9 +391,9 @@ mod tests {
                                 ]
                                 .into_iter(),
                             )),
-                            block: block_filter.clone(),
-                            extrinsic: extrinsic_filter.clone(),
-                            event: event_filter.clone(),
+                            block: block_filter,
+                            extrinsic: extrinsic_filter,
+                            event: event_filter,
                             events: Default::default(),
                             finished: false,
                         };

tests/integration/client.rs

@@ -117,7 +117,7 @@ async fn test_iter() {
         .await
         .unwrap();
     let mut i = 0;
-    while let Some(_) = iter.next().await.unwrap() {
+    while iter.next().await.unwrap().is_some() {
         i += 1;
     }
     assert_eq!(i, 13);