Updates LimitViolationBuilder to reflect changes in LimitViolation#3832
Updates LimitViolationBuilder to reflect changes in LimitViolation#3832Noor-Mustafa123 wants to merge 3 commits intopowsybl:mainfrom
Conversation
|
It seems like you didn't setup the DCO, please see https://github.com/powsybl/powsybl-core/pull/3832/checks?check_run_id=67722763487 for an explanation and how to fix this issue. |
Signed-off-by: nomus <mustafanoor715@gmail.com>
commit is now signed |
NathanDissoubray
left a comment
There was a problem hiding this comment.
That seems good to me apart from a change in tests.
I'm actually wondering if the LimitViolationBuilder should be in LimitViolation instead of a separate class, as I don't see a reason for it to be separate.
I also find it confusing how it's done currently, with both new in LimitViolation and a Builder in separate class.
@olperr1 do you think we should move the builder inside LimitViolation and remove the constructors of LimitViolation ? That would be a breaking change (so only for next release), but I think it'll be better to not have too many ways to make a LimitViolation
...tingency-api/src/test/java/com/powsybl/contingency/violations/LimitViolationBuilderTest.java
Outdated
Show resolved
Hide resolved
…tity Signed-off-by: nomus <mustafanoor715@gmail.com>
|
@NathanDissoubray can you guide me on how ot merge this PR dont have permission currently do i have to ask a maintainer to merge my PR? |
|
@Noor-Mustafa123 one of the maintainer will review your PR (I'm not maintainer), will start the CI if everything seems fine, and if it passes and they don't want any changes, will approve and merge it. |
….java
Please check if the PR fulfills these requirements
Does this PR already have an issue describing the problem?
Fixes #3813
What kind of change does this PR introduce?
Replacement of overloaded constructors with builder in LimitViolation.java
What is the current behavior?
uses overloaded constructors
What is the new behavior (if this is a feature change)?
uses builder pattern
Does this PR introduce a breaking change or deprecate an API?
If yes, please check if the following requirements are fulfilled
What changes might users need to make in their application due to this PR? (migration steps)
Other information: