-
Notifications
You must be signed in to change notification settings - Fork 219
refactor(#1960): Refactor transformation rules, add schema restore #1994
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
Conversation
|
|
||
| @Override | ||
| public int getRulePriority() { | ||
| return 100; |
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.
Can we manage priorities somewhere centrally? E.g. via an enum?
With the current way it's hard to assess the individual priority value
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.
If this is not possible, I would suggest that we add a small explanation somewhere in the JavaDoc (e.g. at the interface) that briefly explains the concept of rulePriority.
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.
thanks, I modified the code to provide all priorities from an enum
|
|
||
| @Override | ||
| public void visit(CreateNestedRuleDescription rule) { | ||
|
|
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.
Is there a reason why all the visit methods are emtpy?
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 added a comment to the empty methods to clearly state that the operation is skipped for stateless rules
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.
Can we make the methods in this class none static? This is currently not critical, but if we change this, the clients that use the class should be easier to test. This comment also applies to the other Helper and Utils classes. We can also have a discussion about this.
|
|
||
| @Override | ||
| public int getRulePriority() { | ||
| return 100; |
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.
If this is not possible, I would suggest that we add a small explanation somewhere in the JavaDoc (e.g. at the interface) that briefly explains the concept of rulePriority.
Purpose
Refactor adapter schema transformation logic:
getRulePriorityto each transformation rule which determines the order of rule application, which was previously done implicitely.Remarks
PR introduces (a) breaking change(s): no
PR introduces (a) deprecation(s): no