-
Notifications
You must be signed in to change notification settings - Fork 21
Conversation
import java.util.Arrays; | ||
import java.util.List; | ||
|
||
import static org.fusesource.jansi.Ansi.ansi; | ||
import java.util.function.BiConsumer; |
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 used the same formatter than Quarkus ... should I revert this and use another? Don't want to create unnecessary changes everywhere.
If there is no a decision made, I think we should follow the Quarkus formatter and enforce that these tests follow the rules. What do you think?
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's an import sort Maven plugin configured in this project, mvn compile
should apply I think?
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 don't mean only about sort import, but general formatter rules. This is the formatter I think we should follow: https://github.com/quarkusio/quarkus/tree/master/independent-projects/ide-config/src/main/resources
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.
yup, formater inclusion is on our todo, last time @Ladicek had some objections to Quarkus formatter, so we didn't push hard on it. But as more people will be looking into the project, it might be time to push it :)
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.
The Eclipse formatter is crap and more often than not, I have to fight it to leave the code at least a little readable. I hate it with a passion, but feel free to use it on new code :-)
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.
Whole codebase would get (re)formatted, I would go with https://github.com/quarkusio/quarkus/blob/master/build-parent/pom.xml#L459 way not to need to worry about everyone's IDE setup
common/src/main/java/io/quarkus/ts/openshift/common/Command.java
Outdated
Show resolved
Hide resolved
common/src/main/java/io/quarkus/ts/openshift/common/OpenShiftTestExtension.java
Outdated
Show resolved
Hide resolved
078f43c
to
5c1e18c
Compare
import java.util.stream.Stream; | ||
|
||
import static org.fusesource.jansi.Ansi.ansi; | ||
import static org.junit.platform.commons.util.AnnotationUtils.findAnnotatedFields; | ||
|
||
// TODO at this point, this class is close to becoming unreadable, and could use some refactoring | ||
// TODO at this point, this class is close to becoming unreadable, and could use some refactoring. | ||
// Raised https://github.com/quarkus-qe/quarkus-openshift-test-suite/issues/108 for the refactoring. |
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.
As stated in the comment, I've raised the #108 issue to do this.
...c/main/java/io/quarkus/ts/openshift/common/actions/CopyLogsOnOpenShiftFailureActionImpl.java
Outdated
Show resolved
Hide resolved
Just one comment, otherwise LGTM. |
5c1e18c
to
656a348
Compare
...c/main/java/io/quarkus/ts/openshift/common/actions/CopyLogsOnOpenShiftFailureActionImpl.java
Outdated
Show resolved
Hide resolved
656a348
to
a13d668
Compare
Changes: