#87 Grouping stack traces into single log statements for sub-processes #90
Conversation
|
@dean-e-clark Thanks, I will ask someone to do a code review here, thanks for your pull request |
|
@mkordas this pull request is for you, please review |
|
@mkordas I see this failed the CI builds for styling reasons. Is there a comprehensive list of the styling rules somewhere? |
|
@dean-e-clark I'll help you today :) |
|
@dean-e-clark first of all, PR description must contain link to the issue, can you fix it? |
| @@ -107,6 +107,16 @@ | |||
| private transient boolean closed; | |||
|
|
|||
| /** | |||
| * Maximum number of log lines for a stack trace | |||
| */ | |||
| public static final int DEFUALT_MAX_LENGTH = 1000; | |||
There was a problem hiding this comment.
@dean-e-clark public static final should not be used at all: http://www.yegor256.com/2015/07/06/public-static-literals.html
There is no one list, it's just set of hundreds of various checks done by Qulice |
| @@ -420,6 +444,34 @@ private static void close(final Closeable res) { | |||
| this.output = out; | |||
| this.level = lvl; | |||
| } | |||
|
|
|||
| /** | |||
| * Checks if line is part of a stack trace and should be appended. | |||
There was a problem hiding this comment.
@dean-e-clark does it mean that stactraces can have just these 3 prefixes and otherwise they are not stacktraces?
There was a problem hiding this comment.
@mkordas It's not what they start with, but it's what comes after the initial line
There was a problem hiding this comment.
@dean-e-clark OK, thanks for explaining
| /** | ||
| * Example application to help test {@link VerboseProcess}. | ||
| * @author Dean Clark ([email protected]) | ||
| * @version $Id$ |
There was a problem hiding this comment.
@dean-e-clark we also need @since tag here
| * Provides all captured logging events. | ||
| * @return Copy of log list | ||
| */ | ||
| public final List<LoggingEvent> getLogs() { |
There was a problem hiding this comment.
@dean-e-clark we cannot have any getters in code: http://www.yegor256.com/2014/09/16/getters-and-setters-are-evil.html
| try { | ||
| process.stdout(); | ||
| } catch (final IllegalArgumentException ex) { | ||
| failed = true; |
There was a problem hiding this comment.
@dean-e-clark instead of 3 lines around faled we could have just one line with Assert.fail just after process.stdout();
| ); | ||
| MatcherAssert.assertThat( | ||
| ex.getMessage(), | ||
| Matchers.containsString(VerboseProcessExample.SYSOUT_2) |
There was a problem hiding this comment.
@dean-e-clark these assertions could be joined, Hamcrest supports composition
There was a problem hiding this comment.
@mkordas I'll give that a go, but I have a feeling it will result in another PMD violation or something
There was a problem hiding this comment.
@dean-e-clark why you think so? There shouldn't be any problems with it :)
|
@dean-e-clark see my comments, it's getting better and better 😃 |
| } else { | ||
| finalpath = String.format("%s%s", rootpath, "/bin/java"); | ||
| } | ||
| final File file = new File(finalpath); |
There was a problem hiding this comment.
@dean-e-clark this variable can be inlined as well
There was a problem hiding this comment.
@mkordas that would make the require duplicated nested-ifs
There was a problem hiding this comment.
@dean-e-clark I don't see how inlining could cause any problems here, can you please explain?
| * @param input String to strip | ||
| * @return Stripped string | ||
| */ | ||
| private static String stripStart(final String input) { |
There was a problem hiding this comment.
@dean-e-clark this method should be named like a noun, please read http://www.yegor256.com/images/books/elegant-objects/seven-pages.pdf
There was a problem hiding this comment.
@mkordas removed this in favor of String.trim()
There was a problem hiding this comment.
@dean-e-clark I don't see your changes, can you please push them?
|
@dean-e-clark how is it going here? There are still some comments open |
|
@dean-e-clark I've responded to some your questions, but still many comments are open - let's finish this one finally :) |
|
@dean-e-clark do you want to finish this one? |
|
@dean-e-clark ping |
|
@mkordas Sorry, on an extended vacation until the 28th. Will try to find some time |
|
@dean-e-clark it's 29th today, will you find some time this week? Not much is left I suppose :) |
|
@mkordas Yep! Catching up on some other work tasks, but I should be able to push the
final revisions tomorrow
|
Merging from upstream to get header changes
|
@mkordas AppVeyor seems to be broken, but I've made the other requested changes and the Travis CI build looks good |
|
@mkordas You still around? |
|
@mkordas happy new year! really exciting to see the work on this issue. wondering if we're close to merging the fix in? |
Proposed solution to #87
It will group typical Java stack traces into single log statements rather than one log statement per line.