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

refactor main core + initial setup for tap regulator input/output #565

Merged
merged 11 commits into from
Apr 9, 2024

Conversation

mgovers
Copy link
Member

@mgovers mgovers commented Apr 5, 2024

Relates to #74
Relates to #557

I needed this work for #557 but I deemed it out of scope of that PR, so I cherry-picked the changes and applied them here.

@nitbharambe maybe this will help you with #562 as well

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers added the improvement Improvement on internal implementation label Apr 5, 2024
@mgovers mgovers self-assigned this Apr 5, 2024
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers changed the title refactor main core + implement tap input/output refactor main core + initial setup for tap regulator input/output Apr 5, 2024
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
Copy link
Member Author

mgovers commented Apr 8, 2024

Quality Gate Failed Quality Gate failed

Failed conditions 69.6% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

  • most sonar cloud warnings are not new issues
  • the ones that are, I deem out of scope of this PR because they would make the changes less obvious. may be future iterations.
  • coverage relates to tap regulator input/output (to be tested in a separate PR), and fallback code like exceptions

Copy link
Contributor

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo left a comment

Choose a reason for hiding this comment

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

Some are comments on existing codes prior to this PR.

mgovers and others added 5 commits April 8, 2024 10:52
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Copy link

sonarcloud bot commented Apr 9, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
70.6% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@Jerry-Jinfeng-Guo
Copy link
Contributor

Not sure if any new tests are needed for the code coverage

@mgovers
Copy link
Member Author

mgovers commented Apr 9, 2024

Not sure if any new tests are needed for the code coverage

not really necessary. there are no tests specifically about I/O currently in the first place. coverage will be reached once main model integration tests + validation tests are created

@mgovers
Copy link
Member Author

mgovers commented Apr 9, 2024

BTW: pulling main into this branch is not required anymore due to the existence of the merge queue. it is recommended if the changes in main are big or conflicts are expected, but that's not the case here.

@mgovers mgovers enabled auto-merge April 9, 2024 07:40
@Jerry-Jinfeng-Guo
Copy link
Contributor

BTW: pulling main into this branch is not required anymore due to the existence of the merge queue. it is recommended if the changes in main are big or conflicts are expected, but that's not the case here.

That is noice. I was under the impression only conflicts between branchs in queue were addressed with this.

@mgovers mgovers added this pull request to the merge queue Apr 9, 2024
@mgovers
Copy link
Member Author

mgovers commented Apr 9, 2024

BTW: pulling main into this branch is not required anymore due to the existence of the merge queue. it is recommended if the changes in main are big or conflicts are expected, but that's not the case here.

That is noice. I was under the impression only conflicts between branchs in queue were addressed with this.

that was the case until tony and I unchecked the Require branch up to date merge check 😬 Note that if there are conflicts, it will still require you to do the merge manually

@Jerry-Jinfeng-Guo
Copy link
Contributor

BTW: pulling main into this branch is not required anymore due to the existence of the merge queue. it is recommended if the changes in main are big or conflicts are expected, but that's not the case here.

That is noice. I was under the impression only conflicts between branchs in queue were addressed with this.

that was the case until tony and I unchecked the Require branch up to date merge check 😬 Note that if there are conflicts, it will still require you to do the merge manually

So long story short: do it anyways...

Merged via the queue into main with commit 1bec1dc Apr 9, 2024
25 of 26 checks passed
@mgovers mgovers deleted the feature/tap-regulator-input-output branch April 9, 2024 08:22
@mgovers
Copy link
Member Author

mgovers commented Apr 9, 2024

BTW: pulling main into this branch is not required anymore due to the existence of the merge queue. it is recommended if the changes in main are big or conflicts are expected, but that's not the case here.

That is noice. I was under the impression only conflicts between branchs in queue were addressed with this.

that was the case until tony and I unchecked the Require branch up to date merge check 😬 Note that if there are conflicts, it will still require you to do the merge manually

So long story short: do it anyways...

only if there's a good reason 😬

@mgovers mgovers mentioned this pull request Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement on internal implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants