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

[IIDM v1.6.0] Make substations optional (#365) #414

Merged
merged 8 commits into from
Feb 11, 2022

Conversation

sebalaig
Copy link
Contributor

@sebalaig sebalaig commented Jan 7, 2022

Please check if the PR fulfills these requirements (please use '[x]' to check the checkboxes, or submit the PR and then click the checkboxes)

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Does this PR already have an issue describing the problem ? If so, link to this issue using '#XXX' and skip the rest
Closes #365

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change or deprecate an API? If yes, check the following:

  • The Breaking Change or Deprecated label has been added
  • The migration guide has been updated in the github wiki (What changes might users need to make in their application due to this PR?)

Other information:

(if any of the questions/checkboxes don't apply, please delete them entirely)

@sebalaig sebalaig self-assigned this Jan 7, 2022
@sebalaig sebalaig linked an issue Jan 7, 2022 that may be closed by this pull request
@mathbagu mathbagu force-pushed the update-version-1.6.0 branch from 4e20298 to aecb4fe Compare January 12, 2022 13:06
Base automatically changed from update-version-1.6.0 to integration/v1.6.0 January 12, 2022 13:58
Signed-off-by: Sébastien LAIGRE <[email protected]>
Signed-off-by: Sébastien LAIGRE <[email protected]>
@sebalaig sebalaig force-pushed the issue365-substation-optional branch from c67f461 to 6648d8b Compare January 21, 2022 14:59
}

Network& ThreeWindingsTransformerAdder::getNetwork() {
return m_substation.getNetwork();
return const_cast<Network&>(static_cast<const ThreeWindingsTransformerAdder*>(this)->getNetwork());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice: sonar is not happy with thoses hacks... But I think it is better than duplicating const methods content...

@sebalaig sebalaig requested a review from mathbagu January 21, 2022 15:43
Copy link

@mathbagu mathbagu left a comment

Choose a reason for hiding this comment

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

From my point of view, if we keep both getSubstation and getNullableSubstation, maybe the getSubstation should maybe throw an exception to avoid having an alias countrary at what we have in java.

void VoltageLevel::setNetworkRef(Network& network) {
if (static_cast<bool>(m_networkRef)) {
m_networkRef = network;
}

Choose a reason for hiding this comment

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

To be discuss: I think the networkRef should be settable to null (empty reference) when the voltage is detached from the network

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bug exists in java => I posted powsybl/powsybl-core#1960 and #421 to fix it later

…ow an exception if the substation is not set

Signed-off-by: Sébastien LAIGRE <[email protected]>
Signed-off-by: Mathieu BAGUE <[email protected]>
private:
stdcxx::Reference<Network> m_networkRef;

Choose a reason for hiding this comment

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

Why don't we use NetworkRef instead like we did in Substation?

@@ -262,6 +271,12 @@ VoltageLevel& VoltageLevel::setLowVoltageLimit(double lowVoltageLimit) {
return *this;
}

void VoltageLevel::setNetworkRef(Network& network) {
if (static_cast<bool>(m_networkRef)) {

Choose a reason for hiding this comment

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

Why this test? m_networkRef is always set except if the VoltageLevel is detached. Then, you will never enter this method. Do I miss something?

Mathieu BAGUE added 4 commits February 10, 2022 11:18
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

58.0% 58.0% Coverage
0.0% 0.0% Duplication

@mathbagu mathbagu merged commit 438e8a9 into integration/v1.6.0 Feb 11, 2022
@mathbagu mathbagu deleted the issue365-substation-optional branch February 11, 2022 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make substations optional
2 participants