Uh oh!
There was an error while loading. Please reload this page.
Simplify logging - #233
Conversation
… Maven logging. log message string construction to address.
…unique per test to avoid test lock-ups on Windows due to locked files (also enables parallel testing)
…variable violates thread safety
There was a problem hiding this comment.
funny one, thanks (and for all the other typos too, thanks)
There was a problem hiding this comment.
Could be even shortened to <encoding>UTF-8</encoding> if I remember correctly.
mvitz
commented
Jan 3, 2016
I like most of this PR but (maybe just as a note to myself) this makes the modularisation effort (see #92 for the discussion) even harder as now everything is dependent on maven. |
…plugin-specific exception to address Michael comment about modularization (git-commit-id#92)
autayeu
commented
Jan 3, 2016
Thank you for a thorough review! I've addressed the comments above. Namely, logging bridges now are improved. In addition, I've refactored exception handling to favor modularization, removed few remaining static collection constructors, added project encoding handling to avoid hard-coded charsets. It turns out property="session" removed earlier is needed. This case stresses the need for proper integration testing: I only got this one with manual testing. It should have been done by integration tests. Please, review and merge. Thank you! |
autayeu
commented
Jan 7, 2016
Is there any other outstanding issues with this branch which we need to consider? |
ktoso
commented
Jan 7, 2016
I'll need to find time to review again – we don't want to make the modularisation effort harder, as mentioned by @mvitz. Please bear with me :) |
autayeu
commented
Jan 7, 2016
OK! |
autayeu
commented
Jan 13, 2016
Konrad, please, can you take a look at this pull request? Thank you! |
autayeu
commented
Feb 7, 2016
Konrad, what are the chances of seeing this merged? |
ktoso
commented
Feb 9, 2016
I'll review again today or tomorrow, was traveling and now sick. Thanks for your patience |
autayeu
commented
Feb 9, 2016
Thank you! |
autayeu
commented
Mar 12, 2016
Konrad, how do I interpret the situation with this pull? You're saying you'll merge it, but months later it is still not merged. |
ktoso
commented
Mar 12, 2016
Please don't assume malice where none is intended – i was very sick for a few weeks and then had work travel, was unable to catch up on all things. Thanks for the ping here (that was all that's needed) – will go over it now and merge if all good :) |
ktoso
commented
Mar 12, 2016
Minor nitpicks but no reason to delay merging, thanks again for your patience :) |
autayeu
commented
Mar 12, 2016
Thank you for merging! And for useful review! Now the next step - release ;) |
autayeu
commented
Mar 12, 2016
By the way, your above comments are not nitpicking - there were few things in the pull which were touched by me "passing by", minor refactorings, which strictly speaking should have been separated from this pull. |
| void setVerbose(boolean verbose); | ||
| /* | ||
| The following borrowed from SLF4J Logger.java |
There was a problem hiding this comment.
I had to check license compatibility... seems we're good though - MIT
This pull contains the following changes (in addition to logging simplification itself)