Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore!: energy model type not allowed to change over time #131

Merged
merged 7 commits into from
Aug 2, 2023

Conversation

frodehk
Copy link
Contributor

@frodehk frodehk commented Aug 1, 2023

BREAKING CHANGE: Not possible to change energy model TYPE over time within one consumer.

Why is this pull request needed?

eCalc web is not yet ready to handle results with multiple energy model types, hence it should not be allowed to specify in the yaml model file.

What does this pull request change?

  • Add yaml model file check for multiple energy model types within one consumer. Raise value error if found.
  • Modify some yaml model test files to remove multiple energy model types within one consumer.
  • Update snapshots
  • Add test

Issues related to this change:

https://github.com/equinor/ecalc-engine/issues/4635

@frodehk frodehk requested a review from a team as a code owner August 1, 2023 10:29
@frodehk frodehk self-assigned this Aug 1, 2023
@frodehk frodehk marked this pull request as draft August 1, 2023 10:33
@frodehk frodehk marked this pull request as ready for review August 1, 2023 11:59
Copy link
Contributor

@jorgenengelsen jorgenengelsen 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 to me.

Some comments:

  • Is it needed to document this change? At least in the changelog?
  • Breaking changes should be marked with an exclamation point, like fix!, or feat!

@frodehk frodehk changed the title chore: energy model type not allowed to change over time chore!: energy model type not allowed to change over time Aug 1, 2023
@frodehk
Copy link
Contributor Author

frodehk commented Aug 1, 2023

Looks good to me.

Some comments:

  • Is it needed to document this change? At least in the changelog?
  • Breaking changes should be marked with an exclamation point, like fix!, or feat!

Agree, should documentation be put in a different PR?

@jorgenengelsen
Copy link
Contributor

Looks good to me.
Some comments:

  • Is it needed to document this change? At least in the changelog?
  • Breaking changes should be marked with an exclamation point, like fix!, or feat!

Agree, should documentation be put in a different PR?

Both are fine by me, I have no preference

Copy link
Collaborator

@TeeeJay TeeeJay left a comment

Choose a reason for hiding this comment

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

nice. remember clear changelog, but should hopefully not cause problems, as it "per documentation" shouldnt have been allowed already :)

@frodehk frodehk merged commit 670cff2 into main Aug 2, 2023
4 checks passed
@frodehk frodehk deleted the force-one-energy-model-type-within-consumer branch August 2, 2023 09:03
patnr added a commit to patnr/ecalc that referenced this pull request Aug 21, 2023
* up-main: (65 commits)
  chore(docs): fix equations showing twice (equinor#141)
  chore: fix spelling errors in changelog
  chore(main): release 8.3.0
  chore: add power adjustment constant also for compressor trains with interstage pressure (equinor#136)
  chore: fix typing of fluid composition
  docs: update docs and changelog for energy models (equinor#133)
  chore!: energy model type not allowed to change over time (equinor#131)
  docs: fix generic compressor example
  refactor: move NeqSimfluid creation into NeqSim wrapper
  chore: add 8.3 changelog
  chore: add fluid mixing checks
  ci: update pre-commit settings
  refactor: imrpove naming and documentation
  chore: add fluid mixing checks
  refactor: molar_mass_kg_per_mol is not used in the code
  fix: wrong standard_conditions_density when mixing two fluids
  test: add test for fluid stream mixing
  chore: merge queque
  fix: parse spaces as thousand separators from excel (equinor#107)
  refactor: NeqSim mapping (equinor#120)
  ...
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.

3 participants