Skip to content

Comments

Move CSV-related code to src/input/ (batch 1)#290

Merged
alexdewar merged 10 commits intomainfrom
move-csv-code
Dec 20, 2024
Merged

Move CSV-related code to src/input/ (batch 1)#290
alexdewar merged 10 commits intomainfrom
move-csv-code

Conversation

@alexdewar
Copy link
Collaborator

@alexdewar alexdewar commented Dec 20, 2024

Description

This PR is the first batch of changes for #281. I've split up the following files, putting their contents into submodules under input instead:

  • region.rs
  • agent.rs
  • asset.rs
  • commodity.rs

In most cases, it made sense to split things into further submodules (e.g. there is a separate input::commodity::cost module for dealing with commodity_costs.csv). For assets.rs, I moved the CSV-related code to a submodule under input::agent and incorporated the remainder into agent.rs.

region.rs now only has one silly little struct in it, so it probably doesn't deserve its own .rs file, but I thought I'd leave it there for now.

No functional changes intended.

Closes #287. Closes #282. Closes #283. Closes #284.

Type of change

  • Bug fix (non-breaking change to fix an issue)
  • New feature (non-breaking change to add functionality)
  • Refactoring (non-breaking, non-functional change to improve maintainability)
  • Optimization (non-breaking change to speed up the code)
  • Breaking change (whatever its nature)
  • Documentation (improve or add documentation)

Key checklist

  • All tests pass: $ cargo test
  • The documentation builds and looks OK: $ cargo doc

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@codecov
Copy link

codecov bot commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 96.79144% with 24 lines in your changes missing coverage. Please review.

Project coverage is 95.15%. Comparing base (f8938a9) to head (15850dc).
Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
src/input/agent.rs 94.69% 0 Missing and 6 partials ⚠️
src/agent.rs 66.66% 3 Missing and 1 partial ⚠️
src/input/region.rs 98.21% 1 Missing and 3 partials ⚠️
src/input/agent/objective.rs 97.24% 0 Missing and 3 partials ⚠️
src/input/commodity.rs 91.66% 0 Missing and 3 partials ⚠️
src/input/agent/asset.rs 83.33% 0 Missing and 2 partials ⚠️
src/input/commodity/cost.rs 99.13% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #290      +/-   ##
==========================================
- Coverage   95.17%   95.15%   -0.03%     
==========================================
  Files          13       19       +6     
  Lines        2529     2619      +90     
  Branches     2529     2619      +90     
==========================================
+ Hits         2407     2492      +85     
- Misses         47       50       +3     
- Partials       75       77       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alexdewar alexdewar marked this pull request as ready for review December 20, 2024 10:38
Copy link
Collaborator

@tsmbland tsmbland left a comment

Choose a reason for hiding this comment

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

Looks good! Definitely an improvement. I might have some more comments when everything is done about privacy of functions/structs/objects, but for now everything looks fine.

const AGENT_REGIONS_FILE_NAME: &str = "agent_regions.csv";

#[derive(Debug, Deserialize, PartialEq)]
struct AgentRegion {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This struct should be in the main agent module, no? Since presumably other parts of the code will want to reference this outside the input module

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, just realised that this data is stored as RegionSelection, so I take that back

Still, I think this AgentRegion stuff belongs in its own file in the input.agent module. Might have saved me some confusion

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. I've made a separate submodule for it and a little wrapper function around the generic read_regions_for_entity we use.

agent_id: String,
/// The region to which an agent belongs.
region_id: String,
impl<'de> Deserialize<'de> for SearchSpace {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I kind of think all this deserializing (here and elsewhere) should be part of the input module, and the final struct just stores the already-deserialized object. Does that make sense?

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 did consider this, and Rust does let you separate methods from data structures in this way, but I think to make this work, we'd need a glob import in src/agent.rs (i.e. use crate::input::agent::*;) to get the Deserialize trait for SearchSpace, which we need (struct Agent can't be made Deserialize otherwise). On balance, I'm not sure it's worth it, so I think I'll leave it as is for now.

@alexdewar alexdewar enabled auto-merge December 20, 2024 15:50
@alexdewar
Copy link
Collaborator Author

Thanks for the review @tsmbland! And merry Christmas 🎄

@alexdewar alexdewar merged commit 8423101 into main Dec 20, 2024
5 checks passed
@alexdewar alexdewar deleted the move-csv-code branch December 20, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants