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.

Sign up for GitHub

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

Jump to bottom

fix: json schema accepts MAXIMUM_DISCHARGE_PRESSURE for single speed … #86

Merged
merged 1 commit into from
Jun 23, 2023
Merged

fix: json schema accepts MAXIMUM_DISCHARGE_PRESSURE for single speed … #86

merged 1 commit into from
Jun 23, 2023

Conversation

krisolba1
Copy link
Contributor

…train

when PRESSURE_CONTROL is DOWNSTREAM_CHOKE

Why is this pull request needed?

To fix bug in json schema, that gives wiggly line when using MAXIMUM_DISCHARGE_PRESSURE. This is an allowed property for single speed compressor trains if PRESSURE_CONTROL is set to DOWNSTREAM_CHOKE

@krisolba1 krisolba1 requested a review from a team as a code owner June 21, 2023 12:42
@TeeeJay
Copy link
Collaborator

TeeeJay commented Jun 22, 2023

I guess it is good but one snapshot test is failing... :/ @olelod correct logic?

@olelod
Copy link
Contributor

olelod commented Jun 22, 2023

I guess it is good but one snapshot test is failing... :/ @olelod correct logic?

The logic is correct. Only for single speed compressor trains and only when pressure control is downstream choke.

@krisolba1
fix: json schema accepts MAXIMUM_DISCHARGE_PRESSURE for single speed …
868428d 
…train

when PRESSURE_CONTROL is DOWNSTREAM_CHOKE
TeeeJay
TeeeJay reviewed Jun 23, 2023
View reviewed changes
...nput/validation/snapshots/test_validation_json_schemas/test_json_schema_changed/schemas.json
@@ -2697,6 +2697,20 @@
},
"SINGLE_SPEED_COMPRESSOR_TRAIN": {
"additionalProperties": false,
"else": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

bytte rekkefølge på if og else? har sikkert ikke noe å si i evaluering, men ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha weird, dette som kom ved å kjøre testene med --snapshot-update :P

TeeeJay
TeeeJay approved these changes Jun 23, 2023
View reviewed changes
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.

begynner å bli veldig avansert json schema. jeg er personlig skeptisk, men vi kan ta dette inn og prøve og se om det funker. Blir fort veldig vanskelig/umulig å lese json schema. På et tidspunkt ønsker vi å generere json schema, så spørs om det blir enklere eller vanskeligere eller "umulig" å representere det samme da ...

@krisolba1 krisolba1 merged commit a18de1e into main Jun 23, 2023
4 checks passed
@krisolba1 krisolba1 deleted the fix/issue-4606 branch June 23, 2023 09:59
@github-actions github-actions bot mentioned this pull request Jun 23, 2023
Merged
krisolba1 added a commit that referenced this pull request Jun 26, 2023
@krisolba1
fix: json schema accepts MAXIMUM_DISCHARGE_PRESSURE for single speed …
30d487c 
…train (#86)

when PRESSURE_CONTROL is DOWNSTREAM_CHOKE
krisolba1 added a commit that referenced this pull request Jun 26, 2023
@krisolba1
Patchv8.2.7 (#92)
3fece31 
* fix: json schema accepts MAXIMUM_DISCHARGE_PRESSURE for single speed train (#86)

when PRESSURE_CONTROL is DOWNSTREAM_CHOKE

* fix: json schema allow stages to have control_margin and control_margin_unit (#90)

for SIMPLIFIED_VARIABLE_SPEED_COMPRESSOR_TRAIN
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TeeeJay TeeeJay TeeeJay approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@krisolba1 @TeeeJay @olelod