From 674a7f41bd875cb797eeeefb5e8284588b9b0c20 Mon Sep 17 00:00:00 2001 From: juliancoffee Date: Wed, 2 Jun 2021 17:11:32 +0300 Subject: [PATCH] Using clippy::pedantic and improve asset tests - Turned clippy::pedantic on for loadout_builder.rs and applied some proposition - Check for invalid weights in loadout files --- common/src/comp/inventory/loadout_builder.rs | 193 ++++++++++++------- 1 file changed, 128 insertions(+), 65 deletions(-) diff --git a/common/src/comp/inventory/loadout_builder.rs b/common/src/comp/inventory/loadout_builder.rs index 9dabf59515..edac72a060 100644 --- a/common/src/comp/inventory/loadout_builder.rs +++ b/common/src/comp/inventory/loadout_builder.rs @@ -1,3 +1,11 @@ +#![warn(clippy::pedantic)] +//#![warn(clippy::nursery)] +#![allow( + clippy::cast_precision_loss, + clippy::cast_sign_loss, + clippy::cast_possible_truncation +)] + use crate::{ assets::{self, AssetExt}, comp::{ @@ -19,8 +27,8 @@ use strum_macros::EnumIter; use tracing::warn; /// Builder for character Loadouts, containing weapon and armour items belonging -/// to a character, along with some helper methods for loading Items and -/// ItemConfig +/// to a character, along with some helper methods for loading `Item`-s and +/// `ItemConfig` /// /// ``` /// use veloren_common::{ @@ -76,10 +84,8 @@ fn choose<'a>( ) -> &'a Option { let mut rng = rand::thread_rng(); - items - .choose_weighted(&mut rng, |item| item.0) - .map(|(_p, itemspec)| itemspec) - .unwrap_or_else(|err| match err { + items.choose_weighted(&mut rng, |item| item.0).map_or_else( + |err| match err { WeightedError::NoItem | WeightedError::AllWeightsZero => &None, WeightedError::InvalidWeight => { let err = format!("Negative values of probability in {}.", asset_specifier); @@ -99,7 +105,9 @@ fn choose<'a>( &None } }, - }) + }, + |(_p, itemspec)| itemspec, + ) } #[derive(Debug, Deserialize, Clone)] @@ -110,6 +118,7 @@ impl assets::Asset for LoadoutSpec { const EXTENSION: &'static str = "ron"; } +#[must_use] pub fn make_potion_bag(quantity: u32) -> Item { let mut bag = Item::new_from_asset_expect("common.items.armor.misc.bag.tiny_leather_pouch"); if let Some(i) = bag.slots_mut().iter_mut().next() { @@ -122,6 +131,11 @@ pub fn make_potion_bag(quantity: u32) -> Item { bag } +#[must_use] +// We have many species so this function is long +// Also we are using default tools for un-specified species so +// it's fine to have wildcards +#[allow(clippy::too_many_lines, clippy::match_wildcard_for_single_variants)] pub fn default_main_tool(body: &Body) -> Option { match body { Body::Golem(golem) => match golem.species { @@ -292,12 +306,17 @@ pub fn default_main_tool(body: &Body) -> Option { } } +impl Default for LoadoutBuilder { + fn default() -> Self { Self::new() } +} + impl LoadoutBuilder { - #[allow(clippy::new_without_default)] // TODO: Pending review in #587 + #[must_use] pub fn new() -> Self { Self(Loadout::new_empty()) } + #[must_use] fn with_default_equipment(body: &Body, active_item: Option) -> Self { - let mut builder = LoadoutBuilder::new(); + let mut builder = Self::new(); builder = match body { Body::BipedLarge(biped_large::Body { species: biped_large::Species::Mindflayer, @@ -341,12 +360,22 @@ impl LoadoutBuilder { builder.active_mainhand(active_item) } + #[must_use] pub fn from_asset_expect(asset_specifier: &str) -> Self { - let loadout = LoadoutBuilder::new(); + let loadout = Self::new(); loadout.apply_asset_expect(asset_specifier) } + /// # Usage + /// Creates new `LoadoutBuilder` with all field replaced from + /// `asset_specifier` which corresponds to loadout config + /// + /// # Panics + /// 1) Will panic if there is no asset with such `asset_specifier` + /// 2) Will panic if path to item specified in loadout file doesn't exist + /// 3) Will panic while runs in tests and asset doesn't have "correct" form + #[must_use] pub fn apply_asset_expect(mut self, asset_specifier: &str) -> Self { let spec = LoadoutSpec::load_expect(asset_specifier).read().0.clone(); for (key, specifier) in spec { @@ -354,7 +383,7 @@ impl LoadoutBuilder { ItemSpec::Item(specifier) => Item::new_from_asset_expect(&specifier), ItemSpec::Choice(items) => match choose(&items, asset_specifier) { Some(ItemSpec::Item(item_specifier)) => { - Item::new_from_asset_expect(&item_specifier) + Item::new_from_asset_expect(item_specifier) }, Some(ItemSpec::Choice(_)) => { let err = format!( @@ -426,10 +455,9 @@ impl LoadoutBuilder { EquipSlot::Glider => { self = self.glider(Some(item)); }, - EquipSlot::Armor(slot @ ArmorSlot::Bag1) - | EquipSlot::Armor(slot @ ArmorSlot::Bag2) - | EquipSlot::Armor(slot @ ArmorSlot::Bag3) - | EquipSlot::Armor(slot @ ArmorSlot::Bag4) => { + EquipSlot::Armor( + slot @ (ArmorSlot::Bag1 | ArmorSlot::Bag2 | ArmorSlot::Bag3 | ArmorSlot::Bag4), + ) => { self = self.bag(slot, Some(item)); }, }; @@ -440,9 +468,15 @@ impl LoadoutBuilder { /// Set default armor items for the loadout. This may vary with game /// updates, but should be safe defaults for a new character. + #[must_use] pub fn defaults(self) -> Self { self.apply_asset_expect("common.loadouts.default") } /// Builds loadout of creature when spawned + #[must_use] + // The reason why this function is so long is creating merchant inventory + // with all items to sell. + // Maybe we should do it on the caller side? + #[allow(clippy::too_many_lines)] pub fn build_loadout( body: Body, mut main_tool: Option, @@ -455,7 +489,7 @@ impl LoadoutBuilder { } // Constructs ItemConfig from Item - let active_item = if let Some(ItemKind::Tool(_)) = main_tool.as_ref().map(|i| i.kind()) { + let active_item = if let Some(ItemKind::Tool(_)) = main_tool.as_ref().map(Item::kind) { main_tool } else { Some(Item::empty()) @@ -469,54 +503,63 @@ impl LoadoutBuilder { }); // Creates rest of loadout let loadout_builder = if let Some(config) = config { - use LoadoutConfig::*; - let builder = LoadoutBuilder::new().active_mainhand(active_item); + let builder = Self::new().active_mainhand(active_item); // NOTE: we apply asset after active mainhand so asset has ability override it match config { - Gnarling => match active_tool_kind { - Some(ToolKind::Bow) | Some(ToolKind::Staff) | Some(ToolKind::Spear) => { + LoadoutConfig::Gnarling => match active_tool_kind { + Some(ToolKind::Bow | ToolKind::Staff | ToolKind::Spear) => { builder.apply_asset_expect("common.loadouts.dungeon.tier-0.gnarling") }, _ => builder, }, - Adlet => match active_tool_kind { + LoadoutConfig::Adlet => match active_tool_kind { Some(ToolKind::Bow) => { builder.apply_asset_expect("common.loadouts.dungeon.tier-1.adlet_bow") }, - Some(ToolKind::Spear) | Some(ToolKind::Staff) => { + Some(ToolKind::Spear | ToolKind::Staff) => { builder.apply_asset_expect("common.loadouts.dungeon.tier-1.adlet_spear") }, _ => builder, }, - Sahagin => builder.apply_asset_expect("common.loadouts.dungeon.tier-2.sahagin"), - Haniwa => builder.apply_asset_expect("common.loadouts.dungeon.tier-3.haniwa"), - Myrmidon => builder.apply_asset_expect("common.loadouts.dungeon.tier-4.myrmidon"), - Husk => builder.apply_asset_expect("common.loadouts.dungeon.tier-5.husk"), - Beastmaster => { + LoadoutConfig::Sahagin => { + builder.apply_asset_expect("common.loadouts.dungeon.tier-2.sahagin") + }, + LoadoutConfig::Haniwa => { + builder.apply_asset_expect("common.loadouts.dungeon.tier-3.haniwa") + }, + LoadoutConfig::Myrmidon => { + builder.apply_asset_expect("common.loadouts.dungeon.tier-4.myrmidon") + }, + LoadoutConfig::Husk => { + builder.apply_asset_expect("common.loadouts.dungeon.tier-5.husk") + }, + LoadoutConfig::Beastmaster => { builder.apply_asset_expect("common.loadouts.dungeon.tier-5.beastmaster") }, - Warlord => builder.apply_asset_expect("common.loadouts.dungeon.tier-5.warlord"), - Warlock => builder.apply_asset_expect("common.loadouts.dungeon.tier-5.warlock"), - Villager => builder + LoadoutConfig::Warlord => { + builder.apply_asset_expect("common.loadouts.dungeon.tier-5.warlord") + }, + LoadoutConfig::Warlock => { + builder.apply_asset_expect("common.loadouts.dungeon.tier-5.warlock") + }, + LoadoutConfig::Villager => builder .apply_asset_expect("common.loadouts.village.villager") .bag(ArmorSlot::Bag1, Some(make_potion_bag(10))), - Guard => builder + LoadoutConfig::Guard => builder .apply_asset_expect("common.loadouts.village.guard") .bag(ArmorSlot::Bag1, Some(make_potion_bag(25))), - Merchant => { + LoadoutConfig::Merchant => { let mut backpack = Item::new_from_asset_expect("common.items.armor.misc.back.backpack"); let mut coins = economy - .map(|e| e.unconsumed_stock.get(&Good::Coin)) - .flatten() + .and_then(|e| e.unconsumed_stock.get(&Good::Coin)) .copied() .unwrap_or_default() .round() .min(rand::thread_rng().gen_range(1000.0..3000.0)) as u32; let armor = economy - .map(|e| e.unconsumed_stock.get(&Good::Armor)) - .flatten() + .and_then(|e| e.unconsumed_stock.get(&Good::Armor)) .copied() .unwrap_or_default() / 10.0; @@ -541,8 +584,7 @@ impl LoadoutBuilder { "common.items.armor.misc.bag.reliable_backpack", ); let weapon = economy - .map(|e| e.unconsumed_stock.get(&Good::Tools)) - .flatten() + .and_then(|e| e.unconsumed_stock.get(&Good::Tools)) .copied() .unwrap_or_default() / 10.0; @@ -558,7 +600,7 @@ impl LoadoutBuilder { let mut rng = rand::thread_rng(); let mut item_with_amount = |item_id: &str, amount: &mut f32| { if *amount > 0.0 { - let mut item = Item::new_from_asset_expect(&item_id); + let mut item = Item::new_from_asset_expect(item_id); // NOTE: Conversion to and from f32 works fine because we make sure the // number we're converting is ≤ 100. let max = amount.min(16.min(item.max_amount()) as f32) as u32; @@ -577,8 +619,7 @@ impl LoadoutBuilder { "common.items.armor.misc.bag.reliable_backpack", ); let mut ingredients = economy - .map(|e| e.unconsumed_stock.get(&Good::Ingredients)) - .flatten() + .and_then(|e| e.unconsumed_stock.get(&Good::Ingredients)) .copied() .unwrap_or_default() / 10.0; @@ -597,8 +638,7 @@ impl LoadoutBuilder { // food for sale at every site, to be used until we have some solution like NPC // houses as a limit on econsim population growth let mut food = economy - .map(|e| e.unconsumed_stock.get(&Good::Food)) - .flatten() + .and_then(|e| e.unconsumed_stock.get(&Good::Food)) .copied() .unwrap_or_default() .max(10000.0) @@ -612,8 +652,7 @@ impl LoadoutBuilder { "common.items.armor.misc.bag.reliable_backpack", ); let mut potions = economy - .map(|e| e.unconsumed_stock.get(&Good::Potions)) - .flatten() + .and_then(|e| e.unconsumed_stock.get(&Good::Potions)) .copied() .unwrap_or_default() / 10.0; @@ -634,107 +673,127 @@ impl LoadoutBuilder { }, } } else { - LoadoutBuilder::with_default_equipment(&body, active_item) + Self::with_default_equipment(&body, active_item) }; Self(loadout_builder.build()) } + #[must_use] pub fn active_mainhand(mut self, item: Option) -> Self { self.0.swap(EquipSlot::ActiveMainhand, item); self } + #[must_use] pub fn active_offhand(mut self, item: Option) -> Self { self.0.swap(EquipSlot::ActiveOffhand, item); self } + #[must_use] pub fn inactive_mainhand(mut self, item: Option) -> Self { self.0.swap(EquipSlot::InactiveMainhand, item); self } + #[must_use] pub fn inactive_offhand(mut self, item: Option) -> Self { self.0.swap(EquipSlot::InactiveOffhand, item); self } + #[must_use] pub fn shoulder(mut self, item: Option) -> Self { self.0.swap(EquipSlot::Armor(ArmorSlot::Shoulders), item); self } + #[must_use] pub fn chest(mut self, item: Option) -> Self { self.0.swap(EquipSlot::Armor(ArmorSlot::Chest), item); self } + #[must_use] pub fn belt(mut self, item: Option) -> Self { self.0.swap(EquipSlot::Armor(ArmorSlot::Belt), item); self } + #[must_use] pub fn hands(mut self, item: Option) -> Self { self.0.swap(EquipSlot::Armor(ArmorSlot::Hands), item); self } + #[must_use] pub fn pants(mut self, item: Option) -> Self { self.0.swap(EquipSlot::Armor(ArmorSlot::Legs), item); self } + #[must_use] pub fn feet(mut self, item: Option) -> Self { self.0.swap(EquipSlot::Armor(ArmorSlot::Feet), item); self } + #[must_use] pub fn back(mut self, item: Option) -> Self { self.0.swap(EquipSlot::Armor(ArmorSlot::Back), item); self } + #[must_use] pub fn ring1(mut self, item: Option) -> Self { self.0.swap(EquipSlot::Armor(ArmorSlot::Ring1), item); self } + #[must_use] pub fn ring2(mut self, item: Option) -> Self { self.0.swap(EquipSlot::Armor(ArmorSlot::Ring2), item); self } + #[must_use] pub fn neck(mut self, item: Option) -> Self { self.0.swap(EquipSlot::Armor(ArmorSlot::Neck), item); self } + #[must_use] pub fn lantern(mut self, item: Option) -> Self { self.0.swap(EquipSlot::Lantern, item); self } + #[must_use] pub fn glider(mut self, item: Option) -> Self { self.0.swap(EquipSlot::Glider, item); self } + #[must_use] pub fn head(mut self, item: Option) -> Self { self.0.swap(EquipSlot::Armor(ArmorSlot::Head), item); self } + #[must_use] pub fn tabard(mut self, item: Option) -> Self { self.0.swap(EquipSlot::Armor(ArmorSlot::Tabard), item); self } + #[must_use] pub fn bag(mut self, which: ArmorSlot, item: Option) -> Self { self.0.swap(EquipSlot::Armor(which), item); self } + #[must_use] pub fn build(self) -> Loadout { self.0 } } @@ -774,14 +833,14 @@ mod tests { ]; for config in LoadoutConfig::iter() { - test_weapons.iter().for_each(|test_weapon| { - LoadoutBuilder::build_loadout( + for test_weapon in &test_weapons { + std::mem::drop(LoadoutBuilder::build_loadout( Body::Humanoid(comp::humanoid::Body::random()), Some(Item::new_from_asset_expect(test_weapon)), Some(config), None, - ); - }); + )); + } } } @@ -805,18 +864,18 @@ mod tests { body_type: comp::$species::BodyType::Male, ..body }; - LoadoutBuilder::build_loadout( + std::mem::drop(LoadoutBuilder::build_loadout( Body::$body(female_body), None, None, None, - ); - LoadoutBuilder::build_loadout( + )); + std::mem::drop(LoadoutBuilder::build_loadout( Body::$body(male_body), None, None, None, - ); + )); } }; // recursive call @@ -844,14 +903,11 @@ mod tests { ); } - #[test] - fn test_loadout_asset() { LoadoutBuilder::from_asset_expect("common.loadouts.test"); } - #[test] fn test_all_loadout_assets() { #[derive(Clone)] - struct LoadoutsList(Vec); - impl assets::Compound for LoadoutsList { + struct LoadoutList(Vec); + impl assets::Compound for LoadoutList { fn load( cache: &assets::AssetCache, specifier: &str, @@ -863,7 +919,7 @@ mod tests { .map(|spec| LoadoutSpec::load_cloned(spec)) .collect::>()?; - Ok(LoadoutsList(list)) + Ok(Self(list)) } } @@ -880,16 +936,23 @@ mod tests { ItemSpec::Choice(ref items) => { for item in items { match item { + (p, _) if p <= &0.0 => { + let err = format!( + "Weight is less or equal to 0.0.\n ({:?}: {:?})", + key, specifier, + ); + panic!("\n\n{}\n\n", err); + }, (_, Some(ItemSpec::Item(specifier))) => { - Item::new_from_asset_expect(&specifier); + Item::new_from_asset_expect(specifier); }, (_, None) => {}, (_, _) => { - panic!( - "\n\nChoice of Choice is unimplemented. (Search for \ - \n{:?}: {:#?})\n\n", + let err = format!( + "Choice of Choice is unimplemented. \n({:?}: {:?})", key, specifier, ); + panic!("\n\n{}\n\n", err); }, }; } @@ -898,7 +961,7 @@ mod tests { } } - let loadouts = LoadoutsList::load_expect_cloned("common.loadouts.*").0; + let loadouts = LoadoutList::load_expect_cloned("common.loadouts.*").0; for loadout in loadouts { validate_asset(loadout); }