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

Add caching, sequencing and optional id support on a per-component-basis #820

Merged
merged 14 commits into from
Nov 7, 2024
Merged

Add caching, sequencing and optional id support on a per-component-basis #820

merged 14 commits into from
Nov 7, 2024

Conversation

mgovers
Copy link
Member

@mgovers mgovers commented Nov 6, 2024

Implements step 7i of #548 .

@mgovers mgovers added the feature New feature or request label Nov 6, 2024
@figueroa1395 figueroa1395 mentioned this pull request Nov 6, 2024
Merged
@mgovers
fix missing support for mixed optional ids
f24eac3 
Signed-off-by: Martijn Govers <[email protected]>
@mgovers
minor revert
c6345c1 
Signed-off-by: Martijn Govers <[email protected]>
@mgovers
cleanup
2e2da4b 
Signed-off-by: Martijn Govers <[email protected]>
@mgovers
more cleanup
925d7bf 
Signed-off-by: Martijn Govers <[email protected]>
@mgovers
more cleanup
963792e 
Signed-off-by: Martijn Govers <[email protected]>
@mgovers
more cleanup
6fc9294 
Signed-off-by: Martijn Govers <[email protected]>
@mgovers mgovers force-pushed the bug/fix-mixed-optional-id branch from 5c4d3cc to 6fc9294 Compare November 6, 2024 14:35
@mgovers
minor cleanup
e8939f8 
Signed-off-by: Martijn Govers <[email protected]>
@mgovers mgovers marked this pull request as ready for review November 6, 2024 14:40
@mgovers
make ci great again
86493ae 
Signed-off-by: Martijn Govers <[email protected]>
@figueroa1395 figueroa1395 mentioned this pull request Nov 6, 2024
Merged
4 tasks
@mgovers
fix ci
4255b4d 
Signed-off-by: Martijn Govers <[email protected]>
@mgovers mgovers force-pushed the bug/fix-mixed-optional-id branch from 518454c to 4255b4d Compare November 6, 2024 15:19
mgovers and others added 3 commits November 6, 2024 16:23
figueroa1395
figueroa1395 reviewed Nov 6, 2024
View reviewed changes
Copy link
Contributor

@figueroa1395 figueroa1395 left a comment

Choose a reason for hiding this comment

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

I still haven't reviewed the c++ tests (will do tomorrow morning), but the rest is reviewed. It looks good, and the tests pass! Most comments are mostly for my understanding.

power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_core/update.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_core/update.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Outdated Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Show resolved Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/main_model_impl.hpp Show resolved Hide resolved
@mgovers
resolve comments
b454043 
Signed-off-by: Martijn Govers <[email protected]>
figueroa1395
figueroa1395 reviewed Nov 7, 2024
View reviewed changes
Copy link
Contributor

@figueroa1395 figueroa1395 left a comment

Choose a reason for hiding this comment

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

Finished reviewing the (remaining) tests, with only some minor comments. Looks good to me.

tests/native_api_tests/test_api_model.cpp Show resolved Hide resolved
tests/native_api_tests/test_api_model.cpp Show resolved Hide resolved
tests/native_api_tests/test_api_model.cpp Outdated Show resolved Hide resolved
tests/native_api_tests/test_api_model.cpp Outdated Show resolved Hide resolved
@mgovers
resolve comments
33e613c 
Signed-off-by: Martijn Govers <[email protected]>
@mgovers mgovers merged commit 916400f into bug/additional-tests-optional-ids Nov 7, 2024
24 checks passed
@mgovers mgovers deleted the bug/fix-mixed-optional-id branch November 7, 2024 12:34
mgovers added a commit that referenced this pull request Nov 7, 2024
@mgovers
Merge pull request #820 from PowerGridModel/bug/fix-mixed-optional-id
408d8df 
Add caching, sequencing and optional id support on a per-component-basis

Signed-off-by: Martijn Govers <[email protected]>
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Nov 7, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

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

@nitbharambe nitbharambe nitbharambe left review comments

@figueroa1395 figueroa1395 figueroa1395 left review comments

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo left review comments

Assignees
No one assigned
Labels
feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mgovers @Jerry-Jinfeng-Guo @figueroa1395 @nitbharambe