-
Notifications
You must be signed in to change notification settings - Fork 2
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
Implement version parsing #20
Conversation
…ore and api versions to 2.14.0.
@lukacupic thank you for your Pull Request. I'll assign someone to review it soon. |
@amihaiemil I couldn't find any assignee for this task. This is either because there are no contributors with role Please, make sure there is at least one available contributor with the required role and the project can afford to pay them. |
I wasn't sure if I should be writing tests for this one, but now that the coverage has dropped I'm guessing I should. |
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.
@lukacupic It's ok for a first version see my comment. Many thanks!
* | ||
* @param args Command line arguments | ||
*/ | ||
private static void initOptions(final String[] args) { |
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.
@lukacupic Logic in this method should probably be pulled out in a proper object which will be an Iterable of String (each option being a String).
But we can do this in a future task. Then, we will also be able to write unit tests for 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.
Yes, as more options are added, a proper object should replace the method. Feel free to open this up as an issue if you already have some options in mind.
@rultor merge it please |
@amihaiemil OK, I'll try to merge now. You can check the progress of the merge here |
@amihaiemil Done! FYI, the full log is here (took me 2min) |
PR for #19
I also bumped log4j's versions (both core and api) to 2.14.0 because I was having some errors with the previous (lower) versions.