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

Add rustfmt rules to the project. #575

Merged
merged 6 commits into from
Sep 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,20 @@ env:
REGISTRY: ghcr.io

jobs:
rustfmt:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Install latest nightly
uses: actions-rs/toolchain@v1
with:
toolchain: nightly
override: true
components: rustfmt

- name: Rustfmt check
run: cargo +nightly fmt --all -- --check

lint-toml-files:
runs-on: ubuntu-latest
steps:
Expand Down Expand Up @@ -71,12 +85,11 @@ jobs:
needs:
- lint-toml-files
- prevent-openssl
- rustfmt
runs-on: ubuntu-latest
strategy:
matrix:
include:
- command: fmt
args: --all --verbose -- --check
- command: clippy
args: --all-targets --all-features
- command: make
Expand Down
8 changes: 8 additions & 0 deletions .rustfmt.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
max_width = 90 # changed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't reducing from the default 100 to 90 cause a lot of wrapping? I already feel a bit constrained with the default 100, also considering that almost everyone uses wide screens

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using only a laptop=) The default value is 120. I didn't try 100 by I know that with 90, it always fits into GitHub split mode review=)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normalize_comments = true # changed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is cool, but isn't stable yet:

rust-lang/rustfmt#3350

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many fields from here are unstable=) It is why it is nightly rust fmt

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. Does this require users to switch between nightly and stable to run cargo fmt? The codebase itself builds with stable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They need to do cargo +nightly fmt. But all IDE support a separate channel for rustfmt.
изображение

imports_layout = "Vertical" # changed
imports_granularity = "Crate" # changed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally prefer Module. Things get a bit confusing, maybe even unreadable, if we have multiple nested levels - whereas they are most of the times just single lines if its per module

https://rust-lang.github.io/rustfmt/?version=v1.5.1&search=#Module%5C%3A

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example from our codebase("Crate"):
изображение

Example from our codebase("Module"):
изображение

I'm okay with both variants but prefer more "Crate" because I can easy collapse imports from one crate or add a cfg flag for whole crate=)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the cfg flag can go both ways. For example, it is also likely that a cfg flag will affect a subset of imports within a crate, rather than the entire crate.

use crate::{ThingA, ThingB}
#[cfg(feature = "feature-c")]
use crate::ThingC

Will rustfmt avoid collapsing the import if we have a cfg flag on a specific set of imports from a crate?

Copy link
Collaborator Author

@xgreenx xgreenx Aug 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will not touch cfg imports. Also, if you have an "enter" between to import section, it will not merge them into one, but it will collapse and sort all inside each section(the same for cfg).

Example with "enter":
image

Example without "enter":
image

trailing_semicolon = false # changed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is very good

edition = "2021" # changed
use_try_shorthand = true # changed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is cool too, but try is so old that we don't even see it anymore. Anyway, its definitely something we should have

use_field_init_shorthand = true # changed
14 changes: 9 additions & 5 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ We use an RFC process to maintain our code standards. They currently live in the

## Building and setting up a development workspace

Fuel Core is mostly written in Rust, but includes components written in C++ (RocksDB). We are currently using the latest Rust stable toolchain.
Fuel Core is mostly written in Rust, but includes components written in C++ (RocksDB).
We are currently using the latest Rust stable toolchain to build the project.
But for `rustfmt`, we use Rust nightly toolchain because it provides more code style features(you can check [`rustfmt.toml`](.rustfmt.toml)).

### Prerequisites

Expand All @@ -34,11 +36,12 @@ cd fuel-core

`rustup` is the official toolchain manager for Rust.

We use some additional components such as `rustfmt` and `clippy`, to install those:
We use some additional components such as `clippy` and `rustfmt`(nightly), to install those:

```sh
rustup component add rustfmt
rustup component add clippy
rustup toolchain install nightly
rustup component add rustfmt --toolchain nightly
```

### Building and testing
Expand All @@ -56,14 +59,15 @@ This command will run `cargo build` and also dump the latest schema into `/asset
Linting is done using rustfmt and clippy, which are each separate commands:

```sh
cargo fmt --all --check
cargo +nightly fmt --all --check
```

```sh
cargo clippy --all-targets
```

The test suite follows the Rust cargo standards. The GraphQL service will be instantiated by Tower and will emulate a server/client structure.
The test suite follows the Rust cargo standards. The GraphQL service will be instantiated by
Tower and will emulate a server/client structure.

Testing is simply done using Cargo:

Expand Down
10 changes: 8 additions & 2 deletions fuel-block-importer/src/service.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
use crate::Config;
use fuel_core_interfaces::block_importer::{ImportBlockBroadcast, ImportBlockMpsc};
use fuel_core_interfaces::block_importer::{
ImportBlockBroadcast,
ImportBlockMpsc,
};
use parking_lot::Mutex;
use tokio::{
sync::{broadcast, mpsc},
sync::{
broadcast,
mpsc,
},
task::JoinHandle,
};

Expand Down
10 changes: 8 additions & 2 deletions fuel-block-producer/src/service.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
use crate::Config;
use fuel_core_interfaces::{block_producer::BlockProducerMpsc, txpool};
use fuel_core_interfaces::{
block_producer::BlockProducerMpsc,
txpool,
};
use parking_lot::Mutex;
use tokio::{sync::mpsc, task::JoinHandle};
use tokio::{
sync::mpsc,
task::JoinHandle,
};

pub struct Service {
join: Mutex<Option<JoinHandle<()>>>,
Expand Down
21 changes: 15 additions & 6 deletions fuel-client/build.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,16 @@
use schemafy_lib::{Expander, Schema};
use std::env;
use std::fs::{self, File};
use std::io::prelude::*;
use std::path::PathBuf;
use schemafy_lib::{
Expander,
Schema,
};
use std::{
env,
fs::{
self,
File,
},
io::prelude::*,
path::PathBuf,
};

fn main() {
println!("cargo:rerun-if-changed=./assets/debugAdapterProtocol.json");
Expand All @@ -13,7 +21,8 @@ fn main() {
.expect("Failed to fetch JSON schema");

let json = fs::read_to_string(&path).expect("Failed to parse JSON from schema");
let schema: Schema = serde_json::from_str(&json).expect("Failed to parse Schema from JSON");
let schema: Schema =
serde_json::from_str(&json).expect("Failed to parse Schema from JSON");
let root_name = schema.title.clone().unwrap_or_else(|| "Root".to_owned());

let path = path.into_os_string();
Expand Down
114 changes: 90 additions & 24 deletions fuel-client/src/client.rs
Original file line number Diff line number Diff line change
@@ -1,28 +1,71 @@
use crate::client::schema::contract::ContractBalanceQueryArgs;
use anyhow::Context;
use cynic::{http::SurfExt, Id, MutationBuilder, Operation, QueryBuilder};
use cynic::{
http::SurfExt,
Id,
MutationBuilder,
Operation,
QueryBuilder,
};
use fuel_vm::prelude::*;
use itertools::Itertools;
use schema::{
balance::BalanceArgs,
block::BlockByIdArgs,
coin::{Coin, CoinByIdArgs, SpendQueryElementInput},
contract::{Contract, ContractByIdArgs},
tx::{TxArg, TxIdArgs},
Bytes, ContinueTx, ContinueTxArgs, ConversionError, HexString, IdArg, MemoryArgs, RegisterArgs,
RunResult, SetBreakpoint, SetBreakpointArgs, SetSingleStepping, SetSingleSteppingArgs, StartTx,
StartTxArgs, TransactionId, U64,
coin::{
Coin,
CoinByIdArgs,
SpendQueryElementInput,
},
contract::{
Contract,
ContractByIdArgs,
},
tx::{
TxArg,
TxIdArgs,
},
Bytes,
ContinueTx,
ContinueTxArgs,
ConversionError,
HexString,
IdArg,
MemoryArgs,
RegisterArgs,
RunResult,
SetBreakpoint,
SetBreakpointArgs,
SetSingleStepping,
SetSingleSteppingArgs,
StartTx,
StartTxArgs,
TransactionId,
U64,
};
use std::{
convert::TryInto,
io::{self, ErrorKind},
io::{
self,
ErrorKind,
},
net,
str::{self, FromStr},
str::{
self,
FromStr,
},
};
use types::{
TransactionResponse,
TransactionStatus,
};
use types::{TransactionResponse, TransactionStatus};

use crate::client::schema::tx::DryRunArg;
pub use schema::{PageDirection, PaginatedResult, PaginationRequest};
pub use schema::{
PageDirection,
PaginatedResult,
PaginationRequest,
};

use self::schema::block::ProduceBlockArgs;

Expand Down Expand Up @@ -175,7 +218,12 @@ impl FuelClient {
Ok(self.query(query).await?.register.0 as Word)
}

pub async fn memory(&self, id: &str, start: usize, size: usize) -> io::Result<Vec<u8>> {
pub async fn memory(
&self,
id: &str,
start: usize,
size: usize,
) -> io::Result<Vec<u8>> {
let query = schema::Memory::build(&MemoryArgs {
id: id.into(),
start: start.into(),
Expand Down Expand Up @@ -209,7 +257,11 @@ impl FuelClient {
Ok(())
}

pub async fn set_single_stepping(&self, session_id: &str, enable: bool) -> io::Result<()> {
pub async fn set_single_stepping(
&self,
session_id: &str,
enable: bool,
) -> io::Result<()> {
let operation = SetSingleStepping::build(SetSingleSteppingArgs {
id: Id::new(session_id),
enable,
Expand All @@ -218,7 +270,11 @@ impl FuelClient {
Ok(())
}

pub async fn start_tx(&self, session_id: &str, tx: &Transaction) -> io::Result<RunResult> {
pub async fn start_tx(
&self,
session_id: &str,
tx: &Transaction,
) -> io::Result<RunResult> {
let operation = StartTx::build(StartTxArgs {
id: Id::new(session_id),
tx: serde_json::to_string(tx).expect("Couldn't serialize tx to json"),
Expand Down Expand Up @@ -314,7 +370,8 @@ impl FuelClient {
}

pub async fn block(&self, id: &str) -> io::Result<Option<schema::block::Block>> {
let query = schema::block::BlockByIdQuery::build(&BlockByIdArgs { id: id.parse()? });
let query =
schema::block::BlockByIdQuery::build(&BlockByIdArgs { id: id.parse()? });

let block = self.query(query).await?.block;

Expand Down Expand Up @@ -389,22 +446,28 @@ impl FuelClient {
}

pub async fn contract(&self, id: &str) -> io::Result<Option<Contract>> {
let query =
schema::contract::ContractByIdQuery::build(ContractByIdArgs { id: id.parse()? });
let query = schema::contract::ContractByIdQuery::build(ContractByIdArgs {
id: id.parse()?,
});
let contract = self.query(query).await?.contract;
Ok(contract)
}

pub async fn contract_balance(&self, id: &str, asset: Option<&str>) -> io::Result<u64> {
pub async fn contract_balance(
&self,
id: &str,
asset: Option<&str>,
) -> io::Result<u64> {
let asset_id: schema::AssetId = match asset {
Some(asset) => asset.parse()?,
None => schema::AssetId::default(),
};

let query = schema::contract::ContractBalanceQuery::build(ContractBalanceQueryArgs {
id: id.parse()?,
asset: asset_id,
});
let query =
schema::contract::ContractBalanceQuery::build(ContractBalanceQueryArgs {
id: id.parse()?,
asset: asset_id,
});

let balance = self.query(query).await.unwrap().contract_balance.amount;
Ok(balance.into())
Expand Down Expand Up @@ -440,7 +503,9 @@ impl FuelClient {
request: PaginationRequest<String>,
) -> io::Result<PaginatedResult<schema::contract::ContractBalance, String>> {
let contract_id: schema::ContractId = contract.parse()?;
let query = schema::contract::ContractBalancesQuery::build(&(contract_id, request).into());
let query = schema::contract::ContractBalancesQuery::build(
&(contract_id, request).into(),
);

let balances = self.query(query).await?.contract_balances.into();

Expand All @@ -452,7 +517,8 @@ impl FuelClient {
owner: Option<&str>,
request: PaginationRequest<String>,
) -> io::Result<PaginatedResult<schema::message::Message, String>> {
let owner: Option<schema::Address> = owner.map(|owner| owner.parse()).transpose()?;
let owner: Option<schema::Address> =
owner.map(|owner| owner.parse()).transpose()?;
let query = schema::message::OwnedMessageQuery::build(&(owner, request).into());

let messages = self.query(query).await?.messages.into();
Expand Down
13 changes: 9 additions & 4 deletions fuel-client/src/client/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,15 @@ pub mod schema {
}

use hex::FromHexError;
use std::array::TryFromSliceError;
use std::fmt::{self, Debug};
use std::io::ErrorKind;
use std::num::TryFromIntError;
use std::{
array::TryFromSliceError,
fmt::{
self,
Debug,
},
io::ErrorKind,
num::TryFromIntError,
};
use thiserror::Error;

pub use primitives::*;
Expand Down
14 changes: 12 additions & 2 deletions fuel-client/src/client/schema/balance.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
use crate::client::schema::{schema, Address, AssetId, PageInfo, U64};
use crate::client::{PageDirection, PaginatedResult, PaginationRequest};
use crate::client::{
schema::{
schema,
Address,
AssetId,
PageInfo,
U64,
},
PageDirection,
PaginatedResult,
PaginationRequest,
};

#[derive(cynic::FragmentArguments, Debug)]
pub struct BalanceArgs {
Expand Down
Loading