Skip to content

Comments

Add validation to SED type commodities#347

Merged
TinyMarsh merged 6 commits intomainfrom
validate_sed_commodities
Jan 28, 2025
Merged

Add validation to SED type commodities#347
TinyMarsh merged 6 commits intomainfrom
validate_sed_commodities

Conversation

@TinyMarsh
Copy link
Collaborator

@TinyMarsh TinyMarsh commented Jan 21, 2025

Description

This pull request focuses on validation of commodities. The most important change includes the implementation of a commodity validation function, and the creation of a corresponding test.

Fixes #167

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 Jan 21, 2025

Codecov Report

Attention: Patch coverage is 98.34711% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.11%. Comparing base (6637473) to head (20af3b0).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/input/process.rs 98.34% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #347      +/-   ##
==========================================
+ Coverage   96.03%   96.11%   +0.08%     
==========================================
  Files          27       27              
  Lines        3279     3400     +121     
  Branches     3279     3400     +121     
==========================================
+ Hits         3149     3268     +119     
  Misses         60       60              
- Partials       70       72       +2     

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

Copy link
Collaborator

@alexdewar alexdewar 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! I've made some minor suggestions.

I would split the actual check into its own separate function to make things a bit more modular and so that you can return early once you've found a producer and consumer.

@TinyMarsh
Copy link
Collaborator Author

Thanks @alexdewar. I've pulled out the checks specific to SED commodities into their own function and implemented your other suggestions.

@TinyMarsh TinyMarsh requested a review from alexdewar January 23, 2025 17:05
Copy link
Collaborator

@alexdewar alexdewar left a comment

Choose a reason for hiding this comment

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

Hey,

Sorry I forgot to re-review this! Looks good. I've made one small suggestion (you can return early from validate_sed_commodity), but other than that, I think we're good to merge.

@TinyMarsh
Copy link
Collaborator Author

Cool. I would have needed to wrap the anyhow! in an Err and return that like:

    return Err(anyhow!(
        "Commodity {} of 'SED' type must have both producer and consumer processes",
        commodity_id
    ));

But it looks like bail! is a shorthand macro for that so I used that instead :) https://docs.rs/anyhow/latest/anyhow/

@TinyMarsh TinyMarsh merged commit b22c45a into main Jan 28, 2025
7 checks passed
@TinyMarsh TinyMarsh deleted the validate_sed_commodities branch January 28, 2025 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

For every commodity of "SED" type, there must be at least one process that consumes the commodity and one process that produces the commodity.

2 participants