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

Network modification connect and disconnect #2831

Merged
merged 24 commits into from
Jan 16, 2024
Merged

Network modification connect and disconnect #2831

merged 24 commits into from
Jan 16, 2024

Conversation

rolnico
Copy link
Member

@rolnico rolnico commented Dec 12, 2023 •
edited
Loading

Please check if the PR fulfills these requirements

  • 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?

Fixes #2805

What kind of change does this PR introduce?

feature

What is the current behavior?

When connecting/disconnecting a terminal, you get a confirmation boolean after the modification is done, which means that if you try to disconnect a connectable with 2 terminals, you will disconnect the first then the second. The problem is that if the 2nd disconnection fails, the first is still done so you have a connectable half-way disconnected

What is the new behavior (if this is a feature change)?
By using the connect/disconnect methods from the connectable class, you will connect/disconnect the different terminals if and only if all the terminals can be connected/disconnected.
If some terminals were already connected/disconnected, it will still work and the other terminals will be connected/disconnected.
If all the terminals were already connected/disconnected, it will fail and return false

Does this PR introduce a breaking change or deprecate an API?

  • Yes
  • No

If yes, please check if the following requirements are fulfilled

  • The Breaking Change or Deprecated label has been added
  • The migration steps are described in the following section

What changes might users need to make in their application due to this PR? (migration steps)

Other information:

@rolnico
add connect/disconnect in abstractconnectable
7ded9f8 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
add tests + methods with default parameters + reporter
6963642 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
delete useless lines
86147b4 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
delete useless lines
ddc9725 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico rolnico requested a review from flo-dup December 12, 2023 14:41
@rolnico
factorised code
fe72777 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
added assertion for coverage
f9a629a 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
added network modification for disconnections
5af0b7f 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
added network modification for connections
9dcd0c3 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
blank line missing
d1dd45e 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
flo-dup
flo-dup reviewed Dec 19, 2023
View reviewed changes
iidm/iidm-api/src/main/java/com/powsybl/iidm/network/Connectable.java Outdated Show resolved Hide resolved
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/AbstractConnectable.java Show resolved Hide resolved
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/AbstractConnectable.java Show resolved Hide resolved
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/AbstractConnectable.java Outdated Show resolved Hide resolved
iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/AbstractConnectable.java
Comment on lines +310 to +313
// Exit if the connectable is already fully disconnected
if (isAlreadyDisconnected) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

true? I would expect the connectable to be non-disconnectable if disconnect returns false. Oh but it's the same for the Terminal... and we didn't put any javadoc to tell what it should return 🙁
If we return true, can't we avoid this? I think the code after should be proof against that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

After some discussion, this breaking change should not be done here.
But could we put in the javadoc that returning false means no changes?

iidm/iidm-impl/src/main/java/com/powsybl/iidm/network/impl/NodeBreakerVoltageLevel.java Outdated Show resolved Hide resolved
iidm/iidm-modification/src/main/java/com/powsybl/iidm/modification/ConnectableConnection.java Outdated Show resolved Hide resolved
iidm/iidm-modification/src/main/java/com/powsybl/iidm/modification/AbstractDisconnection.java Outdated Show resolved Hide resolved
iidm/iidm-modification/src/main/java/com/powsybl/iidm/modification/UnplannedDisconnection.java Outdated Show resolved Hide resolved
@rolnico
use reporter context
7c625ac 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
coverage and duplication
769e707 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
make variables final
0ed6bbc 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
rename variable in switchForConnection
cbfe938 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
removed continue
5aa40b7 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
removed continue
d379da0 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
merged two ifs
55048d2 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
added overrides
2c6015b 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
delete poppedReporter variable
c1e87d2 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
use of abstractDisconnection logger
62d90f1 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
key modification
393748e 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
Merge branch 'main' into nro/network_modif_connect
6001a6e
@rolnico
update IIDM version in test files
a5bf6e3 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
delete debug line
7dc40ad 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@rolnico
modification in reporter and logger
4905e6d 
Signed-off-by: Nicolas Rol <nicolas.rol@rte-france.com>
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
97.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@flo-dup flo-dup merged commit 4c577f8 into main Jan 16, 2024
6 checks passed
@flo-dup flo-dup deleted the nro/network_modif_connect branch January 16, 2024 15:27
@flo-dup flo-dup mentioned this pull request May 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flo-dup flo-dup flo-dup approved these changes

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

Successfully merging this pull request may close these issues.

Network modification for connecting/disconnecting a connectable
3 participants
@rolnico @flo-dup @annetill