Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the asset module by extracting the AssetPool and AssetCapacity implementations (and their tests) out of the monolithic src/asset.rs into dedicated submodules, while keeping the public API via re-exports.
Changes:
- Moved
AssetPoolintosrc/asset/pool.rsand re-exported it fromsrc/asset.rs. - Moved
AssetCapacityintosrc/asset/capacity.rsand re-exported it fromsrc/asset.rs. - Added a reusable
asset_divisibletest fixture insrc/fixture.rsto support the extracted tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/asset.rs | Declares capacity/pool submodules and re-exports AssetCapacity/AssetPool; removes inlined implementations/tests. |
| src/asset/pool.rs | New module containing AssetPool implementation and its tests (moved from asset.rs). |
| src/asset/capacity.rs | New module containing AssetCapacity implementation and its tests (moved from asset.rs). |
| src/fixture.rs | Adds asset_divisible fixture used by tests that moved into asset/pool.rs (and still used in asset.rs tests). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/asset/pool.rs
Outdated
| // Decomission unused assets | ||
| let mut decommissioned = Vec::new(); | ||
| asset_pool.decommission_mothballed(2025, mothball_years, &mut decommissioned); | ||
|
|
||
| // Only the removed asset should be decommissioned (since it's not in active pool) | ||
| assert_eq!(asset_pool.assets.len(), 1); // Active pool unchanged |
There was a problem hiding this comment.
These comments don’t match the test setup: the asset is still in the active pool (it was mothballed, not removed), and decommission_mothballed removes it from asset_pool.assets. Please update the commentary to reflect what is actually being asserted.
| // Decomission unused assets | |
| let mut decommissioned = Vec::new(); | |
| asset_pool.decommission_mothballed(2025, mothball_years, &mut decommissioned); | |
| // Only the removed asset should be decommissioned (since it's not in active pool) | |
| assert_eq!(asset_pool.assets.len(), 1); // Active pool unchanged | |
| // Decommission assets that have been mothballed for at least `mothball_years` | |
| let mut decommissioned = Vec::new(); | |
| asset_pool.decommission_mothballed(2025, mothball_years, &mut decommissioned); | |
| // The mothballed asset should be removed from the active pool and added to `decommissioned` | |
| assert_eq!(asset_pool.assets.len(), 1); // One active asset remains |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/asset/pool.rs
Outdated
| if match asset.state { | ||
| AssetState::Commissioned { .. } => !self.assets.contains(&asset), |
There was a problem hiding this comment.
match asset.state attempts to move AssetState out of an Asset behind an Rc (via AssetRef's Deref), which won’t compile for non-Copy AssetState. Match on a reference instead (e.g. match &asset.state / matches!(&asset.state, ...)) to avoid moving out of the shared asset.
| if match asset.state { | |
| AssetState::Commissioned { .. } => !self.assets.contains(&asset), | |
| if match &asset.state { | |
| &AssetState::Commissioned { .. } => !self.assets.contains(&asset), |
There was a problem hiding this comment.
Nope, it is already accessing state by reference (AssetRef implements Deref for Asset).
That said, I don't think the if match is v readable, so I'll tweak it.
| impl Eq for AssetCapacity {} | ||
|
|
||
| impl PartialOrd for AssetCapacity { | ||
| fn partial_cmp(&self, other: &Self) -> Option<Ordering> { | ||
| Some(self.cmp(other)) | ||
| } | ||
| } | ||
|
|
||
| impl Ord for AssetCapacity { | ||
| fn cmp(&self, other: &Self) -> Ordering { | ||
| match (self, other) { | ||
| (AssetCapacity::Continuous(a), AssetCapacity::Continuous(b)) => a.total_cmp(b), | ||
| (AssetCapacity::Discrete(units1, size1), AssetCapacity::Discrete(units2, size2)) => { | ||
| Self::check_same_unit_size(*size1, *size2); | ||
| units1.cmp(units2) | ||
| } | ||
| _ => panic!("Cannot compare different types of AssetCapacity ({self:?} and {other:?})"), | ||
| } |
There was a problem hiding this comment.
AssetCapacity derives PartialEq (float == semantics) but implements Ord using total_cmp, and also declares Eq. This violates the required consistency between Eq/Ord and equality (e.g., 0.0 == -0.0 but total_cmp orders them differently; NaN also breaks reflexivity). Consider implementing PartialEq/Eq manually to align with total_cmp (or drop Ord/Eq if total ordering isn’t required).
There was a problem hiding this comment.
Copilot is on the money here... this could bite us later. I've opened an issue for it: #1162
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1158 +/- ##
=======================================
Coverage 87.48% 87.48%
=======================================
Files 55 57 +2
Lines 7660 7661 +1
Branches 7660 7661 +1
=======================================
+ Hits 6701 6702 +1
Misses 657 657
Partials 302 302 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
The file is getting rather long, so it makes sense to break it up. I've put everything related to
AssetPoolandAssetCapacityinto separate submodules. No functional change intended.Unfortunately, even after this refactoring,
asset.rsis still the longest file in the repo (>1600 lines). I think that probably the cleanest way to break it up further would be to turn some of the methods into standalone functions which take anAssetas an argument and put these into submodules. For example, anything related to costs could be in its own module. This would be a more invasive set of changes though, so I think we should leave it for later (or we can delegate to an LLM 😄).Closes #1117.
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks