-
Notifications
You must be signed in to change notification settings - Fork 43
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
Refactoring XML IIDM serialization classes #2481
Conversation
Signed-off-by: VEDELAGO MIORA <miora.ralambotiana@rte-france.com>
Signed-off-by: VEDELAGO MIORA <miora.ralambotiana@rte-france.com>
iidm/iidm-xml-converter/src/main/java/com/powsybl/iidm/xml/AbstractComplexIdentifiableXml.java
Show resolved
Hide resolved
protected void readUntilEndRootElement(XMLStreamReader reader, XmlUtil.XmlEventHandler eventHandler) throws XMLStreamException { | ||
XmlUtil.readUntilEndElement(getRootElementName(), reader, eventHandler); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some code duplicates, like here, or in read (getting id, name and fictitious), or maybe also readSubElements
. Shouldn't we factorize them? That would lead to some utils method in AbstractIdentifiableXml
. You didn't do it because you didn't want to put any reading in AbstractIdentifiableXml
?
iidm/iidm-xml-converter/src/main/java/com/powsybl/iidm/xml/AbstractComplexIdentifiableXml.java
Show resolved
Hide resolved
iidm/iidm-xml-converter/src/main/java/com/powsybl/iidm/xml/AbstractComplexIdentifiableXml.java
Outdated
Show resolved
Hide resolved
iidm/iidm-xml-converter/src/main/java/com/powsybl/iidm/xml/AbstractComplexIdentifiableXml.java
Outdated
Show resolved
Hide resolved
toApply.forEach(consumer -> consumer.accept(identifiable)); | ||
} | ||
|
||
public final void read(Supplier<A> createAdder, Function<A, T> create, NetworkXmlReaderContext context) throws XMLStreamException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could do as the simple one for creating the adder, that is adding a createAdder(voltageLevel)
, which could therefore remain in AbstractIdentifiableXml
. And then we could also leave in AbstractIdentifiableXml
an abstract read(P, NetworkXmlReaderContext)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot the Function<A, T> create
which is needed. We could do the same, that is, an abstract method which returns the adder. Or, better! in fact we could write adder.add()
as I'd really like to add add
methods in the Adder
s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this PR, if your change in IIDM network is not passed, I will create an abstract method to add but I'm keeping it in mind!
iidm/iidm-xml-converter/src/main/java/com/powsybl/iidm/xml/AliasesXml.java
Outdated
Show resolved
Hide resolved
Signed-off-by: VEDELAGO MIORA <miora.ralambotiana@rte-france.com>
Signed-off-by: VEDELAGO MIORA <miora.ralambotiana@rte-france.com>
Kudos, SonarCloud Quality Gate passed! |
Please check if the PR fulfills these requirements (please use
'[x]'
to check the checkboxes, or submit the PR and then click the checkboxes)Does this PR already have an issue describing the problem ? If so, link to this issue using
'#XXX'
and skip the restNo but it has been discussed in PR about replacing DanglingLine by HalfLine
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Refactoring (XIIDM serialization) to be less strict for complex equipments (shunt compensators and in the future, tie lines)
Does this PR introduce a breaking change or deprecate an API? If yes, check the following:
Might create issues in extensions serialization elsewhere