-
Notifications
You must be signed in to change notification settings - Fork 6
Conversation
@lburgazzoli thanks ! I'm going to make a review soon but wanted to wait until TP2 is finally out before merging. |
No worries :) |
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 ! Looks good and much cleaner than before.
Some comments inline.
@@ -43,6 +43,7 @@ public String toString() { | |||
return "Filter: " + expression + " => " + getSteps(); | |||
} | |||
|
|||
@Override |
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.
Constructor with expression should call super(KIND), too ?
Why is getKind() overwritten here ? (because of the @JsonIgnore
in the super class ?
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.
Leftover leftover :)
ObjectMapper mapper = new ObjectMapper(yamlFactory) | ||
.setSerializationInclusion(JsonInclude.Include.NON_EMPTY) | ||
.enable(SerializationFeature.INDENT_OUTPUT) | ||
.disable(SerializationFeature.WRITE_EMPTY_JSON_ARRAYS) |
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.
WRITE_EMPTY_JSON_ARRAYS is deprecated (but already the default). So maybe just ommit it here ?
return Optional.of(loadFromFile(folder)); | ||
} | ||
return Optional.empty(); | ||
public final class SyndesisHelpers { |
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.
Maybe rename to YamlHelper
(or something else) ?
@@ -28,7 +28,7 @@ | |||
*/ | |||
public static <T> List<T> notNullList(List<T> list) { |
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 inline the whole class ? Its seems to be used only in the ChoiceHandler ....
public class SyndesisAutoConfiguration { | ||
@Autowired | ||
private ApplicationContext context; |
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.
context
is unused.
@rhuss done |
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 !
🎉
This my first PR on syndesis-integration-runtime aimed to remove some funktion leftovers and custom bits.
@syndesisio/backend do you mind carefully review it ?