Merge pull request #62 from OpenVicProject/cleanup

19 files changed, 149 insertions, 179 deletions

diff --git a/src/openvic-simulation/Modifier.cpp b/src/openvic-simulation/Modifier.cpp
index 912b805..d0f3a69 100644
--- a/src/openvic-simulation/Modifier.cpp
+++ b/src/openvic-simulation/Modifier.cpp
@@ -299,13 +299,13 @@ node_callback_t ModifierManager::expect_modifier_value(callback_t<ModifierValue&
 	return expect_modifier_value_and_default(modifier_callback, key_value_invalid_callback);
 }
 
-node_callback_t ModifierManager::expect_whitelisted_modifier_value_and_default(callback_t<ModifierValue&&> modifier_callback, std::set<std::string, std::less<void>> const& whitelist, key_value_callback_t default_callback) const {
+node_callback_t ModifierManager::expect_whitelisted_modifier_value_and_default(callback_t<ModifierValue&&> modifier_callback, string_set_t const& whitelist, key_value_callback_t default_callback) const {
 	return expect_validated_modifier_value_and_default(modifier_callback, default_callback, [&whitelist](ModifierEffect const& effect) -> bool {
 		return whitelist.contains(effect.get_identifier());
 	});
 }
 
-node_callback_t ModifierManager::expect_whitelisted_modifier_value(callback_t<ModifierValue&&> modifier_callback, std::set<std::string, std::less<void>> const& whitelist) const {
+node_callback_t ModifierManager::expect_whitelisted_modifier_value(callback_t<ModifierValue&&> modifier_callback, string_set_t const& whitelist) const {
 	return expect_whitelisted_modifier_value_and_default(modifier_callback, whitelist, key_value_invalid_callback);
 }
 
diff --git a/src/openvic-simulation/Modifier.hpp b/src/openvic-simulation/Modifier.hpp
index c6b15a6..f936e5d 100644
--- a/src/openvic-simulation/Modifier.hpp
+++ b/src/openvic-simulation/Modifier.hpp
@@ -1,7 +1,5 @@
 #pragma once
 
-#include <set>
-
 #include "openvic-simulation/types/IdentifierRegistry.hpp"
 
 namespace OpenVic {
@@ -126,8 +124,8 @@ namespace OpenVic {
 		NodeTools::node_callback_t expect_modifier_value_and_default(NodeTools::callback_t<ModifierValue&&> modifier_callback, NodeTools::key_value_callback_t default_callback) const;
 		NodeTools::node_callback_t expect_modifier_value(NodeTools::callback_t<ModifierValue&&> modifier_callback) const;
 
-		NodeTools::node_callback_t expect_whitelisted_modifier_value_and_default(NodeTools::callback_t<ModifierValue&&> modifier_callback, std::set<std::string, std::less<void>> const& whitelist, NodeTools::key_value_callback_t default_callback) const;
-		NodeTools::node_callback_t expect_whitelisted_modifier_value(NodeTools::callback_t<ModifierValue&&> modifier_callback, std::set<std::string, std::less<void>> const& whitelist) const;
+		NodeTools::node_callback_t expect_whitelisted_modifier_value_and_default(NodeTools::callback_t<ModifierValue&&> modifier_callback, string_set_t const& whitelist, NodeTools::key_value_callback_t default_callback) const;
+		NodeTools::node_callback_t expect_whitelisted_modifier_value(NodeTools::callback_t<ModifierValue&&> modifier_callback, string_set_t const& whitelist) const;
 
 		NodeTools::node_callback_t expect_modifier_value_and_key_map_and_default(NodeTools::callback_t<ModifierValue&&> modifier_callback, NodeTools::key_value_callback_t default_callback, NodeTools::key_map_t&& key_map) const;
 		NodeTools::node_callback_t expect_modifier_value_and_key_map(NodeTools::callback_t<ModifierValue&&> modifier_callback, NodeTools::key_map_t&& key_map) const;
diff --git a/src/openvic-simulation/country/Country.cpp b/src/openvic-simulation/country/Country.cpp
index eadd036..bcc0488 100644
--- a/src/openvic-simulation/country/Country.cpp
+++ b/src/openvic-simulation/country/Country.cpp
@@ -131,7 +131,7 @@ bool CountryManager::load_country_data_file(GameManager& game_manager, std::stri
 	bool ret = expect_dictionary_keys_and_default(
 		[&game_manager, &alternative_colours, &name](std::string_view key, ast::NodeCPtr value) -> bool {
 			const GovernmentType* colour_gov_type;
-			bool ret = game_manager.get_politics_manager().get_government_type_manager().expect_government_type_identifier(assign_variable_callback_pointer(colour_gov_type))(key);
+			bool ret = game_manager.get_politics_manager().get_government_type_manager().expect_government_type_str(assign_variable_callback_pointer(colour_gov_type))(key);
 
 			if (!ret) return false;
 
@@ -161,7 +161,7 @@ bool CountryManager::load_country_data_file(GameManager& game_manager, std::stri
 				[&game_manager, &policies](std::string_view key, ast::NodeCPtr value) -> bool {
 					const Issue* policy;
 					bool ret = expect_identifier_or_string(
-						game_manager.get_politics_manager().get_issue_manager().expect_issue_identifier(
+						game_manager.get_politics_manager().get_issue_manager().expect_issue_str(
 							assign_variable_callback_pointer(policy)
 						)
 					)(value);
@@ -176,7 +176,7 @@ bool CountryManager::load_country_data_file(GameManager& game_manager, std::stri
 				"name", ONE_EXACTLY, expect_identifier_or_string(assign_variable_callback(party_name)),
 				"start_date", ONE_EXACTLY, expect_date(assign_variable_callback(start_date)),
 				"end_date", ONE_EXACTLY, expect_date(assign_variable_callback(end_date)),
-				"ideology", ONE_EXACTLY, expect_identifier_or_string(game_manager.get_politics_manager().get_ideology_manager().expect_ideology_identifier(assign_variable_callback_pointer(ideology)))
+				"ideology", ONE_EXACTLY, expect_identifier_or_string(game_manager.get_politics_manager().get_ideology_manager().expect_ideology_str(assign_variable_callback_pointer(ideology)))
 			)(value);
 
 			country_parties.push_back({ party_name, start_date, end_date, *ideology, std::move(policies) });
diff --git a/src/openvic-simulation/dataloader/Dataloader.cpp b/src/openvic-simulation/dataloader/Dataloader.cpp
index 475cdd1..cfc944d 100644
--- a/src/openvic-simulation/dataloader/Dataloader.cpp
+++ b/src/openvic-simulation/dataloader/Dataloader.cpp
@@ -48,7 +48,7 @@ static constexpr bool path_equals(std::string_view lhs, std::string_view rhs) {
 template<typename T>
 concept is_filename = std::same_as<T, std::filesystem::path> || std::convertible_to<T, std::string_view>;
 
-bool filename_equals(const is_filename auto& lhs, const is_filename auto& rhs) {
+static bool filename_equals(const is_filename auto& lhs, const is_filename auto& rhs) {
 	auto left = [&lhs] {
 		if constexpr (std::same_as<std::decay_t<decltype(lhs)>, std::filesystem::path>)
 			return lhs.filename().string();
diff --git a/src/openvic-simulation/dataloader/NodeTools.cpp b/src/openvic-simulation/dataloader/NodeTools.cpp
index 85bc572..c99a2f0 100644
--- a/src/openvic-simulation/dataloader/NodeTools.cpp
+++ b/src/openvic-simulation/dataloader/NodeTools.cpp
@@ -103,18 +103,20 @@ node_callback_t NodeTools::expect_uint64(callback_t<uint64_t> callback) {
 	);
 }
 
-node_callback_t NodeTools::expect_fixed_point(callback_t<fixed_point_t> callback) {
-	return expect_identifier(
-		[callback](std::string_view identifier) -> bool {
-			bool successful = false;
-			const fixed_point_t val = fixed_point_t::parse(identifier.data(), identifier.length(), &successful);
-			if (successful) {
-				return callback(val);
-			}
-			Logger::error("Invalid fixed point identifier text: ", identifier);
-			return false;
+callback_t<std::string_view> NodeTools::expect_fixed_point_str(callback_t<fixed_point_t> callback) {
+	return [callback](std::string_view identifier) -> bool {
+		bool successful = false;
+		const fixed_point_t val = fixed_point_t::parse(identifier.data(), identifier.length(), &successful);
+		if (successful) {
+			return callback(val);
 		}
-	);
+		Logger::error("Invalid fixed point identifier text: ", identifier);
+		return false;
+	};
+}
+
+node_callback_t NodeTools::expect_fixed_point(callback_t<fixed_point_t> callback) {
+	return expect_identifier(expect_fixed_point_str(callback));
 }
 
 node_callback_t NodeTools::expect_colour(callback_t<colour_t> callback) {
@@ -142,18 +144,20 @@ node_callback_t NodeTools::expect_colour(callback_t<colour_t> callback) {
 	};
 }
 
-node_callback_t NodeTools::expect_date(callback_t<Date> callback) {
-	return expect_identifier(
-		[callback](std::string_view identifier) -> bool {
-			bool successful = false;
-			const Date date = Date::from_string(identifier, &successful);
-			if (successful) {
-				return callback(date);
-			}
-			Logger::error("Invalid date identifier text: ", identifier);
-			return false;
+callback_t<std::string_view> NodeTools::expect_date_str(callback_t<Date> callback) {
+	return [callback](std::string_view identifier) -> bool {
+		bool successful = false;
+		const Date date = Date::from_string(identifier, &successful);
+		if (successful) {
+			return callback(date);
 		}
-	);
+		Logger::error("Invalid date identifier text: ", identifier);
+		return false;
+	};
+}
+
+node_callback_t NodeTools::expect_date(callback_t<Date> callback) {
+	return expect_identifier(expect_date_str(callback));
 }
 
 node_callback_t NodeTools::expect_years(callback_t<Timespan> callback) {
diff --git a/src/openvic-simulation/dataloader/NodeTools.hpp b/src/openvic-simulation/dataloader/NodeTools.hpp
index f06a85f..66b614a 100644
--- a/src/openvic-simulation/dataloader/NodeTools.hpp
+++ b/src/openvic-simulation/dataloader/NodeTools.hpp
@@ -4,6 +4,7 @@
 #include <cstdint>
 #include <functional>
 #include <map>
+#include <set>
 #include <type_traits>
 
 #include <openvic-dataloader/v2script/AbstractSyntaxTree.hpp>
@@ -16,10 +17,13 @@ namespace OpenVic {
 	namespace ast = ovdl::v2script::ast;
 
 	/* Template for map from strings to Ts, in which string_views can be
-	 * searched for without needing to be copied into a string, */
+	 * searched for without needing to be copied into a string */
 	template<typename T>
 	using string_map_t = std::map<std::string, T, std::less<void>>;
 
+	/* String set type supporting heterogeneous key lookup */
+	using string_set_t = std::set<std::string, std::less<void>>;
+
 	namespace NodeTools {
 
 		template<typename Fn, typename Return = void, typename ...Args>
@@ -93,9 +97,11 @@ namespace OpenVic {
 			});
 		}
 
+		callback_t<std::string_view> expect_fixed_point_str(callback_t<fixed_point_t> callback);
 		node_callback_t expect_fixed_point(callback_t<fixed_point_t> callback);
 		node_callback_t expect_colour(callback_t<colour_t> callback);
 
+		callback_t<std::string_view> expect_date_str(callback_t<Date> callback);
 		node_callback_t expect_date(callback_t<Date> callback);
 		node_callback_t expect_years(callback_t<Timespan> callback);
 		node_callback_t expect_months(callback_t<Timespan> callback);
diff --git a/src/openvic-simulation/economy/Building.cpp b/src/openvic-simulation/economy/Building.cpp
index d132e8e..4a99249 100644
--- a/src/openvic-simulation/economy/Building.cpp
+++ b/src/openvic-simulation/economy/Building.cpp
@@ -227,7 +227,7 @@ bool BuildingManager::load_buildings_file(GoodManager const& good_manager, Produ
 		ModifierValue modifier;
 
 		bool ret = modifier_manager.expect_modifier_value_and_keys(move_variable_callback(modifier),
-			"type", ONE_EXACTLY, expect_identifier(expect_building_type_identifier(assign_variable_callback_pointer(type))),
+			"type", ONE_EXACTLY, expect_building_type_identifier(assign_variable_callback_pointer(type)),
 			"on_completion", ZERO_OR_ONE, expect_identifier(assign_variable_callback(on_completion)),
 			"completion_size", ZERO_OR_ONE, expect_fixed_point(assign_variable_callback(completion_size)),
 			"max_level", ONE_EXACTLY, expect_uint(assign_variable_callback(max_level)),
@@ -237,8 +237,8 @@ bool BuildingManager::load_buildings_file(GoodManager const& good_manager, Produ
 			"visibility", ONE_EXACTLY, expect_bool(assign_variable_callback(visibility)),
 			"onmap", ONE_EXACTLY, expect_bool(assign_variable_callback(on_map)),
 			"default_enabled", ZERO_OR_ONE, expect_bool(assign_variable_callback(default_enabled)),
-			"production_type", ZERO_OR_ONE, expect_identifier(production_type_manager.expect_production_type_identifier(
-				assign_variable_callback_pointer(production_type))),
+			"production_type", ZERO_OR_ONE, production_type_manager.expect_production_type_identifier(
+				assign_variable_callback_pointer(production_type)),
 			"pop_build_factory", ZERO_OR_ONE, expect_bool(assign_variable_callback(pop_build_factory)),
 			"strategic_factory", ZERO_OR_ONE, expect_bool(assign_variable_callback(strategic_factory)),
 			"advanced_factory", ZERO_OR_ONE, expect_bool(assign_variable_callback(advanced_factory)),
diff --git a/src/openvic-simulation/economy/ProductionType.cpp b/src/openvic-simulation/economy/ProductionType.cpp
index 3a27cd6..f31ea59 100644
--- a/src/openvic-simulation/economy/ProductionType.cpp
+++ b/src/openvic-simulation/economy/ProductionType.cpp
@@ -178,7 +178,7 @@ bool ProductionTypeManager::add_production_type(PRODUCTION_TYPE_ARGS, GoodManage
 		"type", ZERO_OR_ONE, expect_identifier(expect_mapped_string(type_map, assign_variable_callback(type))), \
 		"workforce", ZERO_OR_ONE, expect_uint(assign_variable_callback(workforce)), \
 		"input_goods", ZERO_OR_ONE, good_manager.expect_good_decimal_map(move_variable_callback(input_goods)), \
-		"output_goods", ZERO_OR_ONE, expect_identifier(good_manager.expect_good_identifier(assign_variable_callback_pointer(output_goods))), \
+		"output_goods", ZERO_OR_ONE, good_manager.expect_good_identifier(assign_variable_callback_pointer(output_goods)), \
 		"value", ZERO_OR_ONE, expect_fixed_point(assign_variable_callback(value)), \
 		"efficiency", ZERO_OR_ONE, good_manager.expect_good_decimal_map(move_variable_callback(efficiency)), \
 		"is_coastal", ZERO_OR_ONE, expect_bool(assign_variable_callback(coastal)), \
diff --git a/src/openvic-simulation/map/Map.cpp b/src/openvic-simulation/map/Map.cpp
index 777579e..13d0933 100644
--- a/src/openvic-simulation/map/Map.cpp
+++ b/src/openvic-simulation/map/Map.cpp
@@ -200,14 +200,6 @@ std::vector<Map::shape_pixel_t> const& Map::get_province_shape_image() const {
 	return province_shape_image;
 }
 
-TerrainTypeManager& Map::get_terrain_type_manager() {
-	return terrain_type_manager;
-}
-
-TerrainTypeManager const& Map::get_terrain_type_manager() const {
-	return terrain_type_manager;
-}
-
 bool Map::add_mapmode(std::string_view identifier, Mapmode::colour_func_t colour_func) {
 	if (identifier.empty()) {
 		Logger::error("Invalid mapmode identifier - empty!");
diff --git a/src/openvic-simulation/map/Map.hpp b/src/openvic-simulation/map/Map.hpp
index 578b471..61b7b48 100644
--- a/src/openvic-simulation/map/Map.hpp
+++ b/src/openvic-simulation/map/Map.hpp
@@ -89,8 +89,7 @@ namespace OpenVic {
 		size_t get_width() const;
 		size_t get_height() const;
 		std::vector<shape_pixel_t> const& get_province_shape_image() const;
-		TerrainTypeManager& get_terrain_type_manager();
-		TerrainTypeManager const& get_terrain_type_manager() const;
+		REF_GETTERS(terrain_type_manager)
 
 		bool add_region(std::string_view identifier, std::vector<std::string_view> const& province_identifiers);
 		IDENTIFIER_REGISTRY_ACCESSORS(region)
diff --git a/src/openvic-simulation/map/TerrainType.cpp b/src/openvic-simulation/map/TerrainType.cpp
index 8624cdb..db910ce 100644
--- a/src/openvic-simulation/map/TerrainType.cpp
+++ b/src/openvic-simulation/map/TerrainType.cpp
@@ -115,7 +115,7 @@ bool TerrainTypeManager::_load_terrain_type_mapping(std::string_view mapping_key
 	bool has_texture = true;
 
 	bool ret = expect_dictionary_keys(
-		"type", ONE_EXACTLY, expect_identifier(expect_terrain_type_identifier(assign_variable_callback_pointer(type))),
+		"type", ONE_EXACTLY, expect_terrain_type_identifier(assign_variable_callback_pointer(type)),
 		"color", ONE_EXACTLY, expect_list_reserve_length(terrain_indicies, expect_uint<TerrainTypeMapping::index_t>(
 			[&terrain_indicies](TerrainTypeMapping::index_t val) -> bool {
 				if (std::find(terrain_indicies.begin(), terrain_indicies.end(), val) == terrain_indicies.end()) {
diff --git a/src/openvic-simulation/military/LeaderTrait.cpp b/src/openvic-simulation/military/LeaderTrait.cpp
index 27f81c5..b90a399 100644
--- a/src/openvic-simulation/military/LeaderTrait.cpp
+++ b/src/openvic-simulation/military/LeaderTrait.cpp
@@ -3,58 +3,51 @@
 using namespace OpenVic;
 using namespace OpenVic::NodeTools;
 
-LeaderTrait::LeaderTrait(std::string_view new_identifier, trait_type_t new_type, ModifierValue new_modifiers)
-    : HasIdentifier { new_identifier }, type { new_type }, modifiers { new_modifiers } {}
+LeaderTrait::LeaderTrait(std::string_view new_identifier, trait_type_t new_type, ModifierValue&& new_modifiers)
+	: HasIdentifier { new_identifier }, type { new_type }, modifiers { std::move(new_modifiers) } {}
 
-trait_type_t LeaderTrait::get_trait_type() const {
-    return type;
+LeaderTrait::trait_type_t LeaderTrait::get_trait_type() const {
+	return type;
 }
 
 bool LeaderTrait::is_personality_trait() const {
-    return type == trait_type_t::PERSONALITY;
+	return type == trait_type_t::PERSONALITY;
 }
 
 bool LeaderTrait::is_background_trait() const {
-    return type == trait_type_t::BACKGROUND;
+	return type == trait_type_t::BACKGROUND;
 }
 
-ModifierValue LeaderTrait::get_modifiers() const {
-    return modifiers;
+ModifierValue const& LeaderTrait::get_modifiers() const {
+	return modifiers;
 }
 
 LeaderTraitManager::LeaderTraitManager() : leader_traits { "leader_traits" } {}
 
-bool LeaderTraitManager::add_leader_trait(std::string_view identifier, trait_type_t type, ModifierValue modifiers) {
-    if (identifier.empty()) {
-        Logger::error("Invalid leader trait identifier - empty!");
-        return false;
-    }
+bool LeaderTraitManager::add_leader_trait(std::string_view identifier, LeaderTrait::trait_type_t type, ModifierValue&& modifiers) {
+	if (identifier.empty()) {
+		Logger::error("Invalid leader trait identifier - empty!");
+		return false;
+	}
 
-    return leader_traits.add_item({ identifier, type, modifiers });
+	return leader_traits.add_item({ identifier, type, std::move(modifiers) });
 }
 
-bool LeaderTraitManager::load_leader_traits_file(ModifierManager& modifier_manager, ast::NodeCPtr root) {
-    bool ret = expect_dictionary_keys(
-        "personality", ONE_EXACTLY, expect_dictionary(
-            [this, &modifier_manager](std::string_view trait_identifier, ast::NodeCPtr value) -> bool {
-                ModifierValue modifiers;
-
-                bool ret = modifier_manager.expect_whitelisted_modifier_value(move_variable_callback(modifiers), allowed_modifiers)(value);
-                ret &= add_leader_trait(trait_identifier, trait_type_t::PERSONALITY, modifiers);
-                return ret;
-            }
-        ),
-        "background", ONE_EXACTLY, expect_dictionary(
-            [this, &modifier_manager](std::string_view trait_identifier, ast::NodeCPtr value) -> bool {
-                ModifierValue modifiers;
-
-                bool ret = modifier_manager.expect_whitelisted_modifier_value(move_variable_callback(modifiers), allowed_modifiers)(value);
-                ret &= add_leader_trait(trait_identifier, trait_type_t::BACKGROUND, modifiers);
-                return ret;
-            }
-        )
-    )(root);
-    lock_leader_traits();
-
-    return ret;
-}
\ No newline at end of file
+bool LeaderTraitManager::load_leader_traits_file(ModifierManager const& modifier_manager, ast::NodeCPtr root) {
+	using enum LeaderTrait::trait_type_t;
+	const auto trait_callback = [this, &modifier_manager](LeaderTrait::trait_type_t type) -> key_value_callback_t {
+		return [this, &modifier_manager, type](std::string_view trait_identifier, ast::NodeCPtr value) -> bool {
+			ModifierValue modifiers;
+			bool ret = modifier_manager.expect_whitelisted_modifier_value(move_variable_callback(modifiers), allowed_modifiers)(value);
+			ret &= add_leader_trait(trait_identifier, type, std::move(modifiers));
+			return ret;
+		};
+	};
+	const bool ret = expect_dictionary_keys(
+		"personality", ONE_EXACTLY, expect_dictionary(trait_callback(PERSONALITY)),
+		"background", ONE_EXACTLY, expect_dictionary(trait_callback(BACKGROUND))
+	)(root);
+	lock_leader_traits();
+
+	return ret;
+}
diff --git a/src/openvic-simulation/military/LeaderTrait.hpp b/src/openvic-simulation/military/LeaderTrait.hpp
index f4445f6..e525e23 100644
--- a/src/openvic-simulation/military/LeaderTrait.hpp
+++ b/src/openvic-simulation/military/LeaderTrait.hpp
@@ -8,65 +8,65 @@
 #include "openvic-simulation/Modifier.hpp"
 
 namespace OpenVic {
-    struct LeaderTraitManager;
+	struct LeaderTraitManager;
 
-    enum class trait_type_t {
-        PERSONALITY,
-        BACKGROUND
-    };
+	struct LeaderTrait : HasIdentifier {
+		friend struct LeaderTraitManager;
 
-    struct LeaderTrait : HasIdentifier {
-        friend struct LeaderTraitManager;
+		enum class trait_type_t {
+			PERSONALITY,
+			BACKGROUND
+		};
 
-    private:
+	private:
 
-        const trait_type_t type;
-        /* 
-         * Allowed modifiers for leaders:
-         * attack - integer
-         * defence - integer
-         * morale - %
-         * organisation - %
-         * reconnaissance - %
-         * speed - %
-         * attrition - %, negative is good
-         * experience - %
-         * reliability - decimal, mil gain or loss for associated POPs
-         */
-        const ModifierValue modifiers;
+		const trait_type_t type;
+		/*
+		 * Allowed modifiers for leaders:
+		 * attack - integer
+		 * defence - integer
+		 * morale - %
+		 * organisation - %
+		 * reconnaissance - %
+		 * speed - %
+		 * attrition - %, negative is good
+		 * experience - %
+		 * reliability - decimal, mil gain or loss for associated POPs
+		 */
+		const ModifierValue modifiers;
 
-        LeaderTrait(std::string_view new_identifier, trait_type_t new_type, ModifierValue new_modifiers);
-        
-    public:
-        LeaderTrait(LeaderTrait&&) = default;
+		LeaderTrait(std::string_view new_identifier, trait_type_t new_type, ModifierValue&& new_modifiers);
 
-        trait_type_t get_trait_type() const;
-        bool is_personality_trait() const;
-        bool is_background_trait() const;
-        ModifierValue get_modifiers() const;
-    };
+	public:
+		LeaderTrait(LeaderTrait&&) = default;
 
-    struct LeaderTraitManager {
-    private:
-        IdentifierRegistry<LeaderTrait> leader_traits;
-        const std::set<std::string, std::less<void>> allowed_modifiers = {
-            "attack",
-            "defence",
-            "morale",
-            "organisation",
-            "reconnaissance",
-            "speed",
-            "attrition",
-            "experience",
-            "reliability"
-        };
-    
-    public:
-        LeaderTraitManager();
+		trait_type_t get_trait_type() const;
+		bool is_personality_trait() const;
+		bool is_background_trait() const;
+		ModifierValue const& get_modifiers() const;
+	};
 
-        bool add_leader_trait(std::string_view identifier, trait_type_t type, ModifierValue modifiers);
-        IDENTIFIER_REGISTRY_ACCESSORS(leader_trait)
+	struct LeaderTraitManager {
+	private:
+		IdentifierRegistry<LeaderTrait> leader_traits;
+		inline static const string_set_t allowed_modifiers = {
+			"attack",
+			"defence",
+			"morale",
+			"organisation",
+			"reconnaissance",
+			"speed",
+			"attrition",
+			"experience",
+			"reliability"
+		};
 
-        bool load_leader_traits_file(ModifierManager& modifier_manager, ast::NodeCPtr root);
-    };
+	public:
+		LeaderTraitManager();
+
+		bool add_leader_trait(std::string_view identifier, LeaderTrait::trait_type_t type, ModifierValue&& modifiers);
+		IDENTIFIER_REGISTRY_ACCESSORS(leader_trait)
+
+		bool load_leader_traits_file(ModifierManager const& modifier_manager, ast::NodeCPtr root);
+	};
 } // namespace OpenVic
diff --git a/src/openvic-simulation/military/Unit.cpp b/src/openvic-simulation/military/Unit.cpp
index 8b010e3..294c77f 100644
--- a/src/openvic-simulation/military/Unit.cpp
+++ b/src/openvic-simulation/military/Unit.cpp
@@ -1,7 +1,5 @@
 #include "Unit.hpp"
 
-#include <set>
-
 #define UNIT_ARGS \
 	icon, sprite, active, unit_type, floating_flag, priority, max_strength, default_organisation, maximum_speed, \
 	weighted_value, move_sound, select_sound, build_time, std::move(build_cost), supply_consumption, std::move(supply_cost)
diff --git a/src/openvic-simulation/politics/Government.cpp b/src/openvic-simulation/politics/Government.cpp
index c65abc6..927c773 100644
--- a/src/openvic-simulation/politics/Government.cpp
+++ b/src/openvic-simulation/politics/Government.cpp
@@ -1,7 +1,5 @@
 #include "Government.hpp"
 
-#include <set>
-
 #include "openvic-simulation/GameManager.hpp"
 
 using namespace OpenVic;
@@ -77,7 +75,7 @@ bool GovernmentTypeManager::load_government_types_file(IdeologyManager const& id
 
 			ret &= expect_dictionary(
 				[this, &ideology_manager, &ideologies, government_type_identifier](std::string_view key, ast::NodeCPtr value) -> bool {
-					static const std::set<std::string, std::less<void>> reserved_keys = {
+					static const string_set_t reserved_keys = {
 						"election", "duration", "appoint_ruling_party", "flagType"
 					};
 					if (reserved_keys.find(key) != reserved_keys.end()) return true;
diff --git a/src/openvic-simulation/pop/Culture.cpp b/src/openvic-simulation/pop/Culture.cpp
index bbfa35a..85c96a0 100644
--- a/src/openvic-simulation/pop/Culture.cpp
+++ b/src/openvic-simulation/pop/Culture.cpp
@@ -1,7 +1,5 @@
 #include "Culture.hpp"
 
-#include <set>
-
 #include "openvic-simulation/dataloader/NodeTools.hpp"
 
 using namespace OpenVic;
@@ -121,7 +119,7 @@ bool CultureManager::_load_culture_group(size_t& total_expected_cultures,
 	bool ret = expect_dictionary_keys_and_default(
 		increment_callback(total_expected_cultures),
 		"leader", ONE_EXACTLY, expect_identifier(assign_variable_callback(leader)),
-		"unit", ZERO_OR_ONE, expect_identifier(expect_graphical_culture_type_identifier(assign_variable_callback_pointer(unit_graphical_culture_type))),
+		"unit", ZERO_OR_ONE, expect_graphical_culture_type_identifier(assign_variable_callback_pointer(unit_graphical_culture_type)),
 		"union", ZERO_OR_ONE, success_callback,
 		"is_overseas", ZERO_OR_ONE, expect_bool(assign_variable_callback(is_overseas))
 	)(culture_group_node);
@@ -193,7 +191,7 @@ bool CultureManager::load_culture_file(ast::NodeCPtr root) {
 			CultureGroup const* culture_group = get_culture_group_by_identifier(culture_group_key);
 			return expect_dictionary(
 				[this, culture_group](std::string_view key, ast::NodeCPtr value) -> bool {
-					static const std::set<std::string, std::less<void>> reserved_keys = {
+					static const string_set_t reserved_keys = {
 						"leader", "unit", "union", "is_overseas"
 					};
 					if (reserved_keys.find(key) != reserved_keys.end()) return true;
diff --git a/src/openvic-simulation/pop/Pop.cpp b/src/openvic-simulation/pop/Pop.cpp
index dc53201..5286393 100644
--- a/src/openvic-simulation/pop/Pop.cpp
+++ b/src/openvic-simulation/pop/Pop.cpp
@@ -123,22 +123,6 @@ bool PopType::get_is_slave() const {
 
 PopManager::PopManager() : pop_types { "pop types" } {}
 
-CultureManager& PopManager::get_culture_manager() {
-	return culture_manager;
-}
-
-CultureManager const& PopManager::get_culture_manager() const {
-	return culture_manager;
-}
-
-ReligionManager& PopManager::get_religion_manager() {
-	return religion_manager;
-}
-
-ReligionManager const& PopManager::get_religion_manager() const {
-	return religion_manager;
-}
-
 bool PopManager::add_pop_type(std::string_view identifier, colour_t colour, PopType::strata_t strata,
 	PopType::sprite_t sprite, Good::good_map_t&& life_needs, Good::good_map_t&& everyday_needs,
 	Good::good_map_t&& luxury_needs, PopType::rebel_units_t&& rebel_units, Pop::pop_size_t max_size,
@@ -237,8 +221,8 @@ bool PopManager::load_pop_into_province(Province& province, std::string_view pop
 	Religion const* religion = nullptr;
 	Pop::pop_size_t size = 0;
 	bool ret = expect_dictionary_keys(
-		"culture", ONE_EXACTLY, expect_identifier(culture_manager.expect_culture_identifier(assign_variable_callback_pointer(culture))),
-		"religion", ONE_EXACTLY, expect_identifier(religion_manager.expect_religion_identifier(assign_variable_callback_pointer(religion))),
+		"culture", ONE_EXACTLY, culture_manager.expect_culture_identifier(assign_variable_callback_pointer(culture)),
+		"religion", ONE_EXACTLY, religion_manager.expect_religion_identifier(assign_variable_callback_pointer(religion)),
 		"size", ONE_EXACTLY, expect_uint(assign_variable_callback(size)),
 		"militancy", ZERO_OR_ONE, success_callback,
 		"rebel_type", ZERO_OR_ONE, success_callback
diff --git a/src/openvic-simulation/pop/Pop.hpp b/src/openvic-simulation/pop/Pop.hpp
index 26f89db..7261831 100644
--- a/src/openvic-simulation/pop/Pop.hpp
+++ b/src/openvic-simulation/pop/Pop.hpp
@@ -99,10 +99,8 @@ namespace OpenVic {
 	public:
 		PopManager();
 
-		CultureManager& get_culture_manager();
-		CultureManager const& get_culture_manager() const;
-		ReligionManager& get_religion_manager();
-		ReligionManager const& get_religion_manager() const;
+		REF_GETTERS(culture_manager)
+		REF_GETTERS(religion_manager)
 
 		bool add_pop_type(std::string_view identifier, colour_t new_colour, PopType::strata_t strata, PopType::sprite_t sprite,
 			Good::good_map_t&& life_needs, Good::good_map_t&& everyday_needs, Good::good_map_t&& luxury_needs,
diff --git a/src/openvic-simulation/types/IdentifierRegistry.hpp b/src/openvic-simulation/types/IdentifierRegistry.hpp
index f8ece3e..70f55ff 100644
--- a/src/openvic-simulation/types/IdentifierRegistry.hpp
+++ b/src/openvic-simulation/types/IdentifierRegistry.hpp
@@ -193,7 +193,7 @@ namespace OpenVic {
 		value_type _const* get_item_by_index(size_t index) _const { \
 			return index < items.size() ? &items[index] : nullptr; \
 		} \
-		NodeTools::callback_t<std::string_view> expect_item_identifier(NodeTools::callback_t<value_type _const&> callback) _const { \
+		NodeTools::callback_t<std::string_view> expect_item_str(NodeTools::callback_t<value_type _const&> callback) _const { \
 			return [this, callback](std::string_view identifier) -> bool { \
 				value_type _const* item = get_item_by_identifier(identifier); \
 				if (item != nullptr) return callback(*item); \
@@ -201,14 +201,12 @@ namespace OpenVic {
 				return false; \
 			}; \
 		} \
+		NodeTools::node_callback_t expect_item_identifier(NodeTools::callback_t<value_type _const&> callback) _const { \
+			return NodeTools::expect_identifier(expect_item_str(callback)); \
+		} \
 		NodeTools::node_callback_t expect_item_dictionary(NodeTools::callback_t<value_type _const&, ast::NodeCPtr> callback) _const { \
 			return NodeTools::expect_dictionary([this, callback](std::string_view key, ast::NodeCPtr value) -> bool { \
-				value_type _const* item = get_item_by_identifier(key); \
-				if (item != nullptr) { \
-					return callback(*item, value); \
-				} \
-				Logger::error("Invalid ", name, " identifier: ", key); \
-				return false; \
+				return expect_item_str(std::bind(callback, std::placeholders::_1, value))(key); \
 			}); \
 		}
 
@@ -309,7 +307,9 @@ GETTERS
 		return plural.get_items(); } \
 	std::vector<std::string_view> get_##singular##_identifiers() const { \
 		return plural.get_item_identifiers(); } \
-	NodeTools::callback_t<std::string_view> expect_##singular##_identifier(NodeTools::callback_t<decltype(plural)::value_type const&> callback) const { \
+	NodeTools::callback_t<std::string_view> expect_##singular##_str(NodeTools::callback_t<decltype(plural)::value_type const&> callback) const { \
+		return plural.expect_item_str(callback); } \
+	NodeTools::node_callback_t expect_##singular##_identifier(NodeTools::callback_t<decltype(plural)::value_type const&> callback) const { \
 		return plural.expect_item_identifier(callback); } \
 	NodeTools::node_callback_t expect_##singular##_dictionary(NodeTools::callback_t<decltype(plural)::value_type const&, ast::NodeCPtr> callback) const { \
 		return plural.expect_item_dictionary(callback); } \
@@ -319,7 +319,9 @@ GETTERS
 #define IDENTIFIER_REGISTRY_NON_CONST_ACCESSORS_CUSTOM_PLURAL(singular, plural) \
 	decltype(plural)::value_type* get_##singular##_by_identifier(std::string_view identifier) { \
 		return plural.get_item_by_identifier(identifier); } \
-	NodeTools::callback_t<std::string_view> expect_##singular##_identifier(NodeTools::callback_t<decltype(plural)::value_type&> callback) { \
+	NodeTools::callback_t<std::string_view> expect_##singular##_str(NodeTools::callback_t<decltype(plural)::value_type&> callback) { \
+		return plural.expect_item_str(callback); } \
+	NodeTools::node_callback_t expect_##singular##_identifier(NodeTools::callback_t<decltype(plural)::value_type&> callback) { \
 		return plural.expect_item_identifier(callback); } \
 	NodeTools::node_callback_t expect_##singular##_dictionary(NodeTools::callback_t<decltype(plural)::value_type&, ast::NodeCPtr> callback) { \
 		return plural.expect_item_dictionary(callback); }