Skip to content

Comments

Check for duplicate flows#353

Closed
tsmbland wants to merge 3 commits intocombine-pacs-flowsfrom
duplicate_flows
Closed

Check for duplicate flows#353
tsmbland wants to merge 3 commits intocombine-pacs-flowsfrom
duplicate_flows

Conversation

@tsmbland
Copy link
Collaborator

@tsmbland tsmbland commented Jan 23, 2025

Description

Adds a check to make sure no process has multiple flows for the same commodity

Fixes #248

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 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.82%. Comparing base (6df67e7) to head (6a0852e).

Additional details and impacted files
@@                  Coverage Diff                   @@
##           combine-pacs-flows     #353      +/-   ##
======================================================
+ Coverage               95.74%   95.82%   +0.07%     
======================================================
  Files                      27       27              
  Lines                    3129     3182      +53     
  Branches                 3129     3182      +53     
======================================================
+ Hits                     2996     3049      +53     
  Misses                     61       61              
  Partials                   72       72              

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

@tsmbland tsmbland requested a review from alexdewar January 23, 2025 14:15
@tsmbland tsmbland linked an issue Jan 23, 2025 that may be closed by this pull request
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.

One small suggestion, but otherwise all good.

I suppose another way to do this would be to turn flows into a HashMap, with commodity IDs as the keys, which would also make it easier to look flows up by commodity ID. We could then check for duplicates when we populate it. But I think that's a job for another day (and would probably conflict with other things like what @TinyMarsh is working on anyway).

Co-authored-by: Alex Dewar <alexdewar@users.noreply.github.com>
@tsmbland tsmbland requested a review from alexdewar January 23, 2025 16:08
@alexdewar alexdewar deleted the branch combine-pacs-flows January 23, 2025 16:30
@alexdewar alexdewar closed this Jan 23, 2025
@alexdewar
Copy link
Collaborator

Oops! I figured it would be easier if I merged #352, but it automatically closed this PR 🤷. I can't seem to edit it to change the target branch to main. Maybe you can @tsmbland?

@tsmbland
Copy link
Collaborator Author

Hmm weird, no I can't seem to. Usually it automatically changes the target branch...

@alexdewar
Copy link
Collaborator

Right? I'm not sure what's happened here... I guess you'll have to make another PR. Sorry 😟

@tsmbland
Copy link
Collaborator Author

tsmbland commented Jan 23, 2025

New (identical) PR - #354

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.

Check that no commodity is both an input and an output of a process

2 participants