ronantakizawa/codereview-bench huggingface dataset:
Repository: paritytech/substrate
File: client/consensus/manual-seal/src/lib.rs (Rust)

Code before:
		create_empty: false,
		finalize: true,
		parent_hash: None,
		sender: None,
	});

	run_manual_seal(ManualSealParams {
		block_import,
		env,
		client,
		pool,
		commands_stream,
		select_chain,
		consensus_data_provider,
		create_inherent_data_providers,
	})
	.await
}

pub async fn run_delayed_finalize<B, CB, C, S>(
	DelayedFinalizeParams {
		client,
		spawn_handle,
		delay_sec,
		_phantom: PhantomData,
	}: DelayedFinalizeParams<B, C, S>,
) where
	B: BlockT + 'static,
	CB: ClientBackend<B> + 'static,
	C: HeaderBackend<B> + Finalizer<B, CB> + ProvideRuntimeApi<B> + BlockchainEvents<B> + 'static,
	S: SpawnNamed,
{
	let mut block_import_stream = client.import_notification_stream();

	while let Some(notification) = block_import_stream.next().await {
		let delay = Delay::new(Duration::from_secs(delay_sec));
		let cloned_client = client.clone();
		spawn_handle.spawn(
			"delayed-finalize",
			None,
			Box::pin(async move {
				delay.await;
				finalize_block(FinalizeBlockParams {
					hash: notification.hash,
					sender: None,
					justification: None,
					finalizer: cloned_client,
					_phantom: PhantomData,
				})
				.await
			}),

Review comment (suggestion):
Some docs on how to use it. A "real" example would be  really nice, but at least saying that you don't set `finalize` in the command stream etc.

Code after:
		finalize: true,
		parent_hash: None,
		sender: None,
	});

	run_manual_seal(ManualSealParams {
		block_import,
		env,
		client,
		pool,
		commands_stream,
		select_chain,
		consensus_data_provider,
		create_inherent_data_providers,
	})
	.await
}

/// Creates a future for delayed finalization of manual sealed blocks.
///
/// The future needs to be spawned in the background alongside the
/// [`run_manual_seal`]/[`run_instant_seal`] future. It is required that
/// [`EngineCommand::SealNewBlock`] is send with `finalize = false` to not finalize blocks directly
/// after building them. This also means that delayed finality can not be used with
/// [`run_instant_seal_and_finalize`].
pub async fn run_delayed_finalize<B, CB, C, S>(
	DelayedFinalizeParams { client, spawn_handle, delay_sec }: DelayedFinalizeParams<C, S>,
) where
	B: BlockT + 'static,
	CB: ClientBackend<B> + 'static,
	C: HeaderBackend<B> + Finalizer<B, CB> + ProvideRuntimeApi<B> + BlockchainEvents<B> + 'static,
	S: SpawnNamed,
{
	let mut block_import_stream = client.import_notification_stream();

	while let Some(notification) = block_import_stream.next().await {
		let delay = Delay::new(Duration::from_secs(delay_sec));
		let cloned_client = client.clone();
		spawn_handle.spawn(
			"delayed-finalize",
			None,
			Box::pin(async move {
				delay.await;
				finalize_block(FinalizeBlockParams {
					hash: notification.hash,
					sender: None,
					justification: None,
					finalizer: cloned_client,
					_phantom: PhantomData,
				})
				.await
Repository: paritytech/substrate
File: client/consensus/manual-seal/src/lib.rs (Rust)

Code before:
		create_empty: false,
		finalize: true,
		parent_hash: None,
		sender: None,
	});

	run_manual_seal(ManualSealParams {
		block_import,
		env,
		client,
		pool,
		commands_stream,
		select_chain,
		consensus_data_provider,
		create_inherent_data_providers,
	})
	.await
}

pub async fn run_delayed_finalize<B, CB, C, S>(
	DelayedFinalizeParams {
		client,
		spawn_handle,
		delay_sec,
		_phantom: PhantomData,
	}: DelayedFinalizeParams<B, C, S>,
) where
	B: BlockT + 'static,
	CB: ClientBackend<B> + 'static,
	C: HeaderBackend<B> + Finalizer<B, CB> + ProvideRuntimeApi<B> + BlockchainEvents<B> + 'static,
	S: SpawnNamed,
{
	let mut block_import_stream = client.import_notification_stream();

	while let Some(notification) = block_import_stream.next().await {
		let delay = Delay::new(Duration::from_secs(delay_sec));
		let cloned_client = client.clone();
		spawn_handle.spawn(
			"delayed-finalize",
			None,
			Box::pin(async move {
				delay.await;
				finalize_block(FinalizeBlockParams {
					hash: notification.hash,
					sender: None,
					justification: None,
					finalizer: cloned_client,
					_phantom: PhantomData,
				})
				.await
			}),

Review comment (suggestion):
```suggestion
	}: DelayedFinalizeParams<C, S>,
```

Code after:
		finalize: true,
		parent_hash: None,
		sender: None,
	});

	run_manual_seal(ManualSealParams {
		block_import,
		env,
		client,
		pool,
		commands_stream,
		select_chain,
		consensus_data_provider,
		create_inherent_data_providers,
	})
	.await
}

/// Creates a future for delayed finalization of manual sealed blocks.
///
/// The future needs to be spawned in the background alongside the
/// [`run_manual_seal`]/[`run_instant_seal`] future. It is required that
/// [`EngineCommand::SealNewBlock`] is send with `finalize = false` to not finalize blocks directly
/// after building them. This also means that delayed finality can not be used with
/// [`run_instant_seal_and_finalize`].
pub async fn run_delayed_finalize<B, CB, C, S>(
	DelayedFinalizeParams { client, spawn_handle, delay_sec }: DelayedFinalizeParams<C, S>,
) where
	B: BlockT + 'static,
	CB: ClientBackend<B> + 'static,
	C: HeaderBackend<B> + Finalizer<B, CB> + ProvideRuntimeApi<B> + BlockchainEvents<B> + 'static,
	S: SpawnNamed,
{
	let mut block_import_stream = client.import_notification_stream();

	while let Some(notification) = block_import_stream.next().await {
		let delay = Delay::new(Duration::from_secs(delay_sec));
		let cloned_client = client.clone();
		spawn_handle.spawn(
			"delayed-finalize",
			None,
			Box::pin(async move {
				delay.await;
				finalize_block(FinalizeBlockParams {
					hash: notification.hash,
					sender: None,
					justification: None,
					finalizer: cloned_client,
					_phantom: PhantomData,
				})
				.await
Repository: paritytech/substrate
File: client/consensus/manual-seal/src/lib.rs (Rust)

Code before:
	pub env: E,

	/// Client instance
	pub client: Arc<C>,

	/// Shared reference to the transaction pool.
	pub pool: Arc<TP>,

	/// SelectChain strategy.
	pub select_chain: SC,

	/// Digest provider for inclusion in blocks.
	pub consensus_data_provider:
		Option<Box<dyn ConsensusDataProvider<B, Proof = P, Transaction = TransactionFor<C, B>>>>,

	/// Something that can create the inherent data providers.
	pub create_inherent_data_providers: CIDP,
}

pub struct DelayedFinalizeParams<C, S> {
	/// Block import instance for well. importing blocks.
	pub client: Arc<C>,

	pub spawn_handle: S,

	/// The delay in seconds before a block is finalized.
	pub delay_sec: u64,
}

/// Creates the background authorship task for the manual seal engine.
pub async fn run_manual_seal<B, BI, CB, E, C, TP, SC, CS, CIDP, P>(
	ManualSealParams {
		mut block_import,
		mut env,
		client,
		pool,
		mut commands_stream,
		select_chain,
		consensus_data_provider,
		create_inherent_data_providers,
	}: ManualSealParams<B, BI, E, C, TP, SC, CS, CIDP, P>,
) where
	B: BlockT + 'static,
	BI: BlockImport<B, Error = sp_consensus::Error, Transaction = sp_api::TransactionFor<C, B>>
		+ Send
		+ Sync
		+ 'static,
	C: HeaderBackend<B> + Finalizer<B, CB> + ProvideRuntimeApi<B> + 'static,
	CB: ClientBackend<B> + 'static,
	E: Environment<B> + 'static,
	E::Proposer: Proposer<B, Proof = P, Transaction = TransactionFor<C, B>>,

Review comment (suggestion):
```suggestion
        /// Handle for spawning delayed finalization tasks
	pub spawn_handle: S,
```

Code after:
	pub env: E,

	/// Client instance
	pub client: Arc<C>,

	/// Shared reference to the transaction pool.
	pub pool: Arc<TP>,

	/// SelectChain strategy.
	pub select_chain: SC,

	/// Digest provider for inclusion in blocks.
	pub consensus_data_provider:
		Option<Box<dyn ConsensusDataProvider<B, Proof = P, Transaction = TransactionFor<C, B>>>>,

	/// Something that can create the inherent data providers.
	pub create_inherent_data_providers: CIDP,
}

/// Params required to start the delayed finalization task.
pub struct DelayedFinalizeParams<C, S> {
	/// Block import instance.
	pub client: Arc<C>,

	/// Handle for spawning delayed finalization tasks.
	pub spawn_handle: S,

	/// The delay in seconds before a block is finalized.
	pub delay_sec: u64,
}

/// Creates the background authorship task for the manually seal engine.
pub async fn run_manual_seal<B, BI, CB, E, C, TP, SC, CS, CIDP, P>(
	ManualSealParams {
		mut block_import,
		mut env,
		client,
		pool,
		mut commands_stream,
		select_chain,
		consensus_data_provider,
		create_inherent_data_providers,
	}: ManualSealParams<B, BI, E, C, TP, SC, CS, CIDP, P>,
) where
	B: BlockT + 'static,
	BI: BlockImport<B, Error = sp_consensus::Error, Transaction = sp_api::TransactionFor<C, B>>
		+ Send
		+ Sync
		+ 'static,
	C: HeaderBackend<B> + Finalizer<B, CB> + ProvideRuntimeApi<B> + 'static,
	CB: ClientBackend<B> + 'static,
Repository: paritytech/substrate
File: frame/support/src/storage/generator/map.rs (Rust)

Code before:

	/// Enumerate all elements in the map.
	fn drain() -> Self::Iterator {
		let mut iterator = Self::iter();
		iterator.drain = true;
		iterator
	}

	fn translate<O: Decode, F: FnMut(K, O) -> Option<V>>(mut f: F) {
		let mut previous_key = None;
		loop {
			previous_key = Self::translate_next(previous_key, &mut f);
			if previous_key.is_none() {
				break
			}
		}
	}

	fn translate_next<O: Decode, F: FnMut(K, O) -> Option<V>>(
		previous_key: Option<Vec<u8>>,
		mut f: F,
	) -> Option<Vec<u8>> {
		let prefix = G::prefix_hash();
		let previous_key = previous_key.unwrap_or_else(|| prefix.clone());

		match sp_io::storage::next_key(&previous_key).filter(|n| n.starts_with(&prefix)) {
			Some(current_key) => {
				let value = match unhashed::get::<O>(&current_key) {
					Some(value) => value,
					None => {
						log::error!("Invalid translate: fail to decode old value");
						return Some(current_key)
					},
				};

				let mut key_material = G::Hasher::reverse(&current_key[prefix.len()..]);
				let key = match K::decode(&mut key_material) {
					Ok(key) => key,
					Err(_) => {
						log::error!("Invalid translate: fail to decode key");
						return Some(current_key)
					},
				};

				match f(key, value) {
					Some(new) => unhashed::put::<V>(&current_key, &new),
					None => unhashed::kill(&current_key),
				}

				Some(current_key)
			},

Review comment (suggestion):
```suggestion
		let current_key =
			sp_io::storage::next_key(&previous_key).filter(|n| n.starts_with(&prefix))?;
```

One less indentation :smile: 

Code after:

	/// Enumerate all elements in the map.
	fn drain() -> Self::Iterator {
		let mut iterator = Self::iter();
		iterator.drain = true;
		iterator
	}

	fn translate<O: Decode, F: FnMut(K, O) -> Option<V>>(mut f: F) {
		let mut previous_key = None;
		loop {
			previous_key = Self::translate_next(previous_key, &mut f);
			if previous_key.is_none() {
				break
			}
		}
	}

	fn translate_next<O: Decode, F: FnMut(K, O) -> Option<V>>(
		previous_key: Option<Vec<u8>>,
		mut f: F,
	) -> Option<Vec<u8>> {
		let prefix = G::prefix_hash();
		let previous_key = previous_key.unwrap_or_else(|| prefix.clone());

		let current_key =
			sp_io::storage::next_key(&previous_key).filter(|n| n.starts_with(&prefix))?;

		let value = match unhashed::get::<O>(&current_key) {
			Some(value) => value,
			None => {
				log::error!("Invalid translate: fail to decode old value");
				return Some(current_key)
			},
		};

		let mut key_material = G::Hasher::reverse(&current_key[prefix.len()..]);
		let key = match K::decode(&mut key_material) {
			Ok(key) => key,
			Err(_) => {
				log::error!("Invalid translate: fail to decode key");
				return Some(current_key)
			},
		};

		match f(key, value) {
			Some(new) => unhashed::put::<V>(&current_key, &new),
			None => unhashed::kill(&current_key),
		}

		Some(current_key)
Repository: paritytech/substrate
File: scripts/ci/node-template-release/src/main.rs (Rust)

Code before:
fn find_cargo_tomls(path: &PathBuf) -> Vec<PathBuf> {
	let path = format!("{}/**/*.toml", path.display());

	let glob = glob::glob(&path).expect("Generates globbing pattern");

	let mut result = Vec::new();
	glob.into_iter().for_each(|file| match file {
		Ok(file) => result.push(file),
		Err(e) => println!("{:?}", e),
	});

	if result.is_empty() {
		panic!("Did not found any `Cargo.toml` files.");
	}

	result
}

/// Parse the given `Cargo.toml`
fn parse_cargo_toml(file: &Path) -> CargoToml {
	let mut content = String::new();
	File::open(file)
		.expect("Cargo.toml exists")
		.read_to_string(&mut content)
		.expect("Reads file");
	content.parse().expect("Cargo.toml is a valid toml file")
}

fn write_cargo_toml(path: &Path, cargo_toml: CargoToml) {
	let mut file = File::create(path).expect(&format!("Creates `{}`.", path.display()));
	write!(file, "{}", cargo_toml).expect("Writes `Cargo.toml`");
}

/// Gets the latest commit id of the repository given by `path`.
fn get_git_commit_id(path: &Path) -> String {
	let repo = git2::Repository::discover(path)
		.expect(&format!("Node template ({}) should be in a git repository.", path.display()));

	let commit_id = repo
		.head()
		.expect("Repository should have a head")
		.peel_to_commit()
		.expect("Head references a commit")
		.id();

	format!("{}", commit_id)
}

/// Rewrites git dependencies:
/// - inserts `workspace = true`;
/// - removes `path`;

Review comment (bug):
```suggestion
	fs::read_to_string(file).expect("Failed to read `Cargo.toml`").parse().expect("Cargo.toml is a valid toml file")
```

Code after:
	let path = format!("{}/**/*.toml", path.display());

	let glob = glob::glob(&path).expect("Generates globbing pattern");

	let mut result = Vec::new();
	glob.into_iter().for_each(|file| match file {
		Ok(file) => result.push(file),
		Err(e) => println!("{:?}", e),
	});

	if result.is_empty() {
		panic!("Did not found any `Cargo.toml` files.");
	}

	result
}

/// Parse the given `Cargo.toml`.
fn parse_cargo_toml(file: &Path) -> CargoToml {
	fs::read_to_string(file)
		.unwrap_or_else(|e| panic!("Failed to read `{}`: {}", file.display(), e))
		.parse()
		.unwrap_or_else(|e| panic!("Failed to parse `{}`: {}", file.display(), e))
}

/// Write the given `Cargo.toml` to the given path.
fn write_cargo_toml(path: &Path, cargo_toml: CargoToml) {
	fs::write(path, cargo_toml.to_string())
		.unwrap_or_else(|e| panic!("Failed to write `{}`: {}", path.display(), e));
}

/// Gets the latest commit id of the repository given by `path`.
fn get_git_commit_id(path: &Path) -> String {
	let repo = git2::Repository::discover(path)
		.expect(&format!("Node template ({}) should be in a git repository.", path.display()));

	let commit_id = repo
		.head()
		.expect("Repository should have a head")
		.peel_to_commit()
		.expect("Head references a commit")
		.id();

	format!("{}", commit_id)
}

/// Rewrites git dependencies:
/// - inserts `workspace = true`;
/// - removes `path`;
/// - removes `version`;
/// - removes `default-features`
Repository: paritytech/substrate
File: scripts/ci/node-template-release/src/main.rs (Rust)

Code before:
	let mut result = Vec::new();
	glob.into_iter().for_each(|file| match file {
		Ok(file) => result.push(file),
		Err(e) => println!("{:?}", e),
	});

	if result.is_empty() {
		panic!("Did not found any `Cargo.toml` files.");
	}

	result
}

/// Parse the given `Cargo.toml`
fn parse_cargo_toml(file: &Path) -> CargoToml {
	let mut content = String::new();
	File::open(file)
		.expect("Cargo.toml exists")
		.read_to_string(&mut content)
		.expect("Reads file");
	content.parse().expect("Cargo.toml is a valid toml file")
}

fn write_cargo_toml(path: &Path, cargo_toml: CargoToml) {
	let mut file = File::create(path).expect(&format!("Creates `{}`.", path.display()));
	write!(file, "{}", cargo_toml).expect("Writes `Cargo.toml`");
}

/// Gets the latest commit id of the repository given by `path`.
fn get_git_commit_id(path: &Path) -> String {
	let repo = git2::Repository::discover(path)
		.expect(&format!("Node template ({}) should be in a git repository.", path.display()));

	let commit_id = repo
		.head()
		.expect("Repository should have a head")
		.peel_to_commit()
		.expect("Head references a commit")
		.id();

	format!("{}", commit_id)
}

/// Rewrites git dependencies:
/// - inserts `workspace = true`;
/// - removes `path`;
/// - removes `version`;
/// - removes `default-features`
/// - and returns the dependencies that were rewritten.
fn update_git_dependencies<F: Copy + Fn(&str) -> bool>(
	cargo_toml: &mut CargoToml,

Review comment (suggestion):
```suggestion
	fs::write(path, cargo_toml.to_string()).expect("Writes `Cargo.toml`");
```

Code after:
	glob.into_iter().for_each(|file| match file {
		Ok(file) => result.push(file),
		Err(e) => println!("{:?}", e),
	});

	if result.is_empty() {
		panic!("Did not found any `Cargo.toml` files.");
	}

	result
}

/// Parse the given `Cargo.toml`.
fn parse_cargo_toml(file: &Path) -> CargoToml {
	fs::read_to_string(file)
		.unwrap_or_else(|e| panic!("Failed to read `{}`: {}", file.display(), e))
		.parse()
		.unwrap_or_else(|e| panic!("Failed to parse `{}`: {}", file.display(), e))
}

/// Write the given `Cargo.toml` to the given path.
fn write_cargo_toml(path: &Path, cargo_toml: CargoToml) {
	fs::write(path, cargo_toml.to_string())
		.unwrap_or_else(|e| panic!("Failed to write `{}`: {}", path.display(), e));
}

/// Gets the latest commit id of the repository given by `path`.
fn get_git_commit_id(path: &Path) -> String {
	let repo = git2::Repository::discover(path)
		.expect(&format!("Node template ({}) should be in a git repository.", path.display()));

	let commit_id = repo
		.head()
		.expect("Repository should have a head")
		.peel_to_commit()
		.expect("Head references a commit")
		.id();

	format!("{}", commit_id)
}

/// Rewrites git dependencies:
/// - inserts `workspace = true`;
/// - removes `path`;
/// - removes `version`;
/// - removes `default-features`
/// - and returns the dependencies that were rewritten.
fn update_git_dependencies<F: Copy + Fn(&str) -> bool>(
	cargo_toml: &mut CargoToml,
	path_filter: F,
) -> Dependencies {
Repository: paritytech/substrate
File: frame/assets/src/lib.rs (Rust)

Code before:
		/// use `refund`.
		///
		/// - `id`: The identifier of the asset for the account holding a deposit.
		/// - `who`: The account to refund.
		///
		/// Emits `Refunded` event when successful.
		#[pallet::call_index(30)]
		#[pallet::weight(T::WeightInfo::refund_other())]
		pub fn refund_other(
			origin: OriginFor<T>,
			id: T::AssetIdParameter,
			who: T::AccountId,
		) -> DispatchResult {
			let origin = ensure_signed(origin)?;
			let id: T::AssetId = id.into();
			Self::do_refund_other(id, &who, &origin)
		}
	}

	/// Implements [AccountTouch] trait.
	/// Note that a depositor can be any account, without any specific privilege.
	/// This implementation is supposed to be used only for creation of system accounts.
	impl<T: Config<I>, I: 'static> AccountTouch<T::AssetId, T::AccountId> for Pallet<T, I> {
		type Balance = DepositBalanceOf<T, I>;

		fn deposit() -> Self::Balance {
			T::AssetAccountDeposit::get()
		}

		fn touch(asset: T::AssetId, who: T::AccountId, depositor: T::AccountId) -> DispatchResult {
			Self::do_touch(asset, who, depositor, true)
		}
	}
}

sp_core::generate_feature_enabled_macro!(runtime_benchmarks_enabled, feature = "runtime-benchmarks", $);


Review comment (suggestion):
```suggestion
		fn deposit_required() -> Self::Balance {
```

Code after:
		/// use `refund`.
		///
		/// - `id`: The identifier of the asset for the account holding a deposit.
		/// - `who`: The account to refund.
		///
		/// Emits `Refunded` event when successful.
		#[pallet::call_index(30)]
		#[pallet::weight(T::WeightInfo::refund_other())]
		pub fn refund_other(
			origin: OriginFor<T>,
			id: T::AssetIdParameter,
			who: T::AccountId,
		) -> DispatchResult {
			let origin = ensure_signed(origin)?;
			let id: T::AssetId = id.into();
			Self::do_refund_other(id, &who, &origin)
		}
	}

	/// Implements [AccountTouch] trait.
	/// Note that a depositor can be any account, without any specific privilege.
	/// This implementation is supposed to be used only for creation of system accounts.
	impl<T: Config<I>, I: 'static> AccountTouch<T::AssetId, T::AccountId> for Pallet<T, I> {
		type Balance = DepositBalanceOf<T, I>;

		fn deposit_required() -> Self::Balance {
			T::AssetAccountDeposit::get()
		}

		fn touch(asset: T::AssetId, who: T::AccountId, depositor: T::AccountId) -> DispatchResult {
			Self::do_touch(asset, who, depositor, true)
		}
	}
}

sp_core::generate_feature_enabled_macro!(runtime_benchmarks_enabled, feature = "runtime-benchmarks", $);
Repository: paritytech/substrate
File: frame/support/procedural/src/pallet/expand/config.rs (Rust)

Code before:
// See the License for the specific language governing permissions and
// limitations under the License.

use crate::pallet::Def;

///
/// * Generate default rust doc
pub fn expand_config(def: &mut Def) -> proc_macro2::TokenStream {
	let config = &def.config;
	let config_item = {
		let item = &mut def.item.content.as_mut().expect("Checked by def parser").1[config.index];
		if let syn::Item::Trait(item) = item {
			item
		} else {
			unreachable!("Checked by config parser")
		}
	};

	config_item.attrs.insert(
		0,
		syn::parse_quote!(
			#[doc = r"
					Configuration trait of this pallet.

					The main purpose of this trait is to act as an interface between this
					pallet, and the runtime in which it is embedded in. A type, function,
					or constant in this trait  is essentially left to be configured by the
					runtime that includes this pallet.

					Consequently, a runtime that wants to include this pallet must
					implement this trait.
			"]
		),
	);

	Default::default()
}


Review comment (suggestion):
```suggestion
					pallet and the runtime in which it is embedded in. A type, function,
```

Code after:
// See the License for the specific language governing permissions and
// limitations under the License.

use crate::pallet::Def;

///
/// * Generate default rust doc
pub fn expand_config(def: &mut Def) -> proc_macro2::TokenStream {
	let config = &def.config;
	let config_item = {
		let item = &mut def.item.content.as_mut().expect("Checked by def parser").1[config.index];
		if let syn::Item::Trait(item) = item {
			item
		} else {
			unreachable!("Checked by config parser")
		}
	};

	config_item.attrs.insert(
		0,
		syn::parse_quote!(
			#[doc = r"Configuration trait of this pallet.

The main purpose of this trait is to act as an interface between this pallet and the runtime in
which it is embedded in. A type, function, or constant in this trait is essentially left to be
configured by the runtime that includes this pallet.

Consequently, a runtime that wants to include this pallet must implement this trait."
			]
		),
	);

	Default::default()
}