Skip to content

Comments

Make process code generic#139

Merged
alexdewar merged 11 commits intomainfrom
make_process_code_generic
Jul 30, 2024
Merged

Make process code generic#139
alexdewar merged 11 commits intomainfrom
make_process_code_generic

Conversation

@alexdewar
Copy link
Collaborator

Description

This PR moves various bits of code out of process.rs and makes them more generic so that they can be used by other modules (e.g. it will be useful for the commodities code).

I've introduced a couple of changes to how things were implemented too:

  1. I'm now using Rc<str> (reference counted strings) for ID values in many places. We end up using a lot of strings as the various process-related input files are read into HashMaps with IDs as strings and we want to avoid copying them as much as possible. An alternative is to use references with appropriate lifetimes but this is a bit more finnicky and doesn't always work (e.g. you can't have a key that refers to a field in the value of the same map).
  2. I changed various functions for reading in and processing CSV data to work with iterators rather than directly with containers (e.g. Vec), which makes the code cleaner and more composable. Some of this has involved writing traits, which seemed odd to me at first, but I think the result is much nicer.

I also fixed #137 while I was at it.

Fixes #137.

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 Jul 29, 2024

Codecov Report

Attention: Patch coverage is 87.30159% with 8 lines in your changes missing coverage. Please review.

Project coverage is 78.85%. Comparing base (b7f3338) to head (7a128db).

Files Patch % Lines
src/input.rs 80.00% 6 Missing ⚠️
src/process.rs 93.93% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #139      +/-   ##
==========================================
- Coverage   80.36%   78.85%   -1.52%     
==========================================
  Files          10       10              
  Lines         163      175      +12     
==========================================
+ Hits          131      138       +7     
- Misses         32       37       +5     

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

@alexdewar alexdewar marked this pull request as draft July 29, 2024 07:24
@alexdewar
Copy link
Collaborator Author

Actually, maybe hold off on reviewing this for now. I've thought of (what might be) a simpler way of doing things

@alexdewar alexdewar marked this pull request as ready for review July 29, 2024 09:25
@alexdewar
Copy link
Collaborator Author

It turned out to be a less simple idea than I thought 😛. I think this is good to go!

@alexdewar
Copy link
Collaborator Author

@tsmbland has signed off on this verbally, so I'm just going to go ahead and merge.

@alexdewar alexdewar merged commit a1e1918 into main Jul 30, 2024
@alexdewar alexdewar deleted the make_process_code_generic branch July 30, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Process erroneously allows for multiple ProcessParameters

1 participant