-
Notifications
You must be signed in to change notification settings - Fork 3
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
Upgrade the Gradle wrapper to 8.7, and the Gradle plugin to 0.7.0 #11
Conversation
That warning has no problems. It means that the code is writing logs by SLF4J, but no actual logging driver is found, then it is logging nothing out. If you wanted to elimiate this warning, and write logs in tests, you may want to include Logback in the test dependencies. |
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.
When making a pull request (or split multiple pull requests), it'd be nice to keep each pull request to pass compile and tests. Unfortunately, this pull request does not compile, and pass the tests.
To keep each pull request to pass compile and tests, you may want to have the check
GitHub Action at first.
Made |
Thanks. After merging #17. I'll modify something and push again. As a result, I expect CI will run. |
d2a5f4e
to
07eec69
Compare
CI should pass in each pull request. It is prioritized than splitting pull requests for readability. If this #11 needs code changes in #13, please consider :
|
Thanks. Allow me to have some time to review this... |
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.
Basically LGTM, then approved, but left a couple of nitpicking comments. Thanks for working on it!
} | ||
} | ||
|
||
@VisibleForTesting | ||
static List<String> buildShell() | ||
protected static List<String> buildShell() |
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.
protected
needed?
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 wanted to ask how to remove the @VisibleForTesting
annotation. Fixed.
Could you tell me how to fix this warning?