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

Adjustment to c api #11

Merged
merged 37 commits into from
Oct 17, 2024
Merged

Adjustment to c api #11

merged 37 commits into from
Oct 17, 2024

Conversation

Laurynas-Jagutis
Copy link
Member

Change to c api

@Laurynas-Jagutis
add worfklow
b74f75c 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
adjust workflow
26810ee 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
change ssh to http for testing purposes
3a6389f 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
adjust workflow
1b8ce27 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
try out brew
800fdd3 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
enable dev build for testing
427a397 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
remove comments
89694c8 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
add new workflow
b813f03 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
delete workflow
e0ccb1c 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
update c api
4e71951 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
format files
bce8295 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis Laurynas-Jagutis added the feature New feature or request label Oct 2, 2024
@Laurynas-Jagutis
add const
e613118 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
fix some clang tidy issues
646863f 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
preallocate
ac4c29f 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
stop some clang tidy checks
5d17092 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
add const, format
cf1a40b 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
fix clang tidy more
570bad9 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
add a nolint statement
10a8517 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
fix clang tidy
f42fb55 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
add const&
ab637af 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
add const
9bb6b50 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
adjust converter, remove build.yml
f8abc0d 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
add const
070a8cc 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
adjust const
fe6b41f 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis Laurynas-Jagutis marked this pull request as ready for review October 7, 2024 08:41
mgovers
mgovers reviewed Oct 7, 2024
View reviewed changes
Copy link
Member

@mgovers mgovers left a comment

Choose a reason for hiding this comment

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

great progress. a couple remarks

.clang-tidy Show resolved Hide resolved
.clang-tidy Show resolved Hide resolved
..._grid_model_io_native/include/power_grid_model_io_native/vnf_converter/vnf_pgm_converter.hpp Outdated Show resolved Hide resolved
power_grid_model_io_native_c/power_grid_model_io_native_c/src/vnf_pgm_converter.cpp Outdated Show resolved Hide resolved
@Laurynas-Jagutis
change metadata to metadatagen
11c4cfc 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
remove reinterpret cast
96928bd 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
add const
7f2763e 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
adjust const
f43114b 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
disable two clang tidy checks since they are failing on the PGM side
80c4a72 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
fix a sonarcloud smell
166ea01 
Signed-off-by: Laurynas Jagutis <[email protected]>
mgovers
mgovers reviewed Oct 7, 2024
View reviewed changes
Copy link
Member

@mgovers mgovers left a comment

Choose a reason for hiding this comment

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

mostly alright. couple minor remarks left

.gitmodules Outdated Show resolved Hide resolved
..._grid_model_io_native/include/power_grid_model_io_native/vnf_converter/vnf_pgm_converter.hpp Outdated Show resolved Hide resolved
...tive_c/power_grid_model_io_native_c/include/power_grid_model_io_native_c/vnf_pgm_converter.h Outdated Show resolved Hide resolved
mgovers
mgovers reviewed Oct 8, 2024
View reviewed changes
Copy link
Member

@mgovers mgovers left a comment

Choose a reason for hiding this comment

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

I'm OK with the PR in the current state, but please add a follow-up PR that adds C++ unit tests + C API tests for the existing functionality.

Basically, we want to do continuous improvements:

  • first, being able to create + call the C API [done]
  • then, being able to create the converter + return a basic string (can be anything) [implemented but untested; this PR]
  • then, doing the same, but from the Python side [next ticket]
  • then, being able to parse nodes [todo]

to prove that we don't break anything along the way and really are moving forward, tests are important.

mgovers
mgovers previously approved these changes Oct 17, 2024
View reviewed changes
Copy link
Member

@mgovers mgovers left a comment

Choose a reason for hiding this comment

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

feel free to do the last 2 comments in a separate PR instead if you prefer

...tive_c/power_grid_model_io_native_c/include/power_grid_model_io_native_c/vnf_pgm_converter.h Outdated Show resolved Hide resolved
power_grid_model_io_native_c/power_grid_model_io_native_c/src/vnf_pgm_converter.cpp Outdated Show resolved Hide resolved
@Laurynas-Jagutis
change delete to destroy
f531881 
Signed-off-by: Laurynas Jagutis <[email protected]>
@Laurynas-Jagutis
change another delete into destroy
fd4a65b 
Signed-off-by: Laurynas Jagutis <[email protected]>
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
3 New issues
0 Accepted issues

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

See analysis details on SonarCloud

@mgovers mgovers added this pull request to the merge queue Oct 17, 2024
Merged via the queue into main with commit 1274760 Oct 17, 2024
25 checks passed
@mgovers mgovers deleted the feature/workflows branch October 17, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgovers mgovers mgovers approved these changes

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.
2 participants
@Laurynas-Jagutis @mgovers