JENKINS-56700 and JENKINS-56701 #153
Conversation
…into jenkins56700
|
@TobiasSchaffner könntest du den PR reviewn? |
Codecov Report
@@ Coverage Diff @@
## master #153 +/- ##
============================================
+ Coverage 88.51% 88.54% +0.03%
- Complexity 1235 1241 +6
============================================
Files 158 158
Lines 3937 3965 +28
Branches 424 431 +7
============================================
+ Hits 3485 3511 +26
Misses 296 296
- Partials 156 158 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Some nitpicking but overall this is looking fine to me. I fear you will have to do something against the lower code coverage to get this approved. Maybe it is a good idea to cut the createIssue method in the GnuFortranParser into pieces for lower complexity and easier testing.
src/main/java/edu/hm/hafner/analysis/parser/GnuFortranParser.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/hm/hafner/analysis/parser/GnuFortranParser.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/hm/hafner/analysis/parser/GnuFortranParser.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/hm/hafner/analysis/parser/GnuFortranParser.java
Outdated
Show resolved
Hide resolved
| // This does not match the whole message, return without issue. | ||
| return Optional.empty(); | ||
| } | ||
| String currentLine = lookahead.peekNext(); |
There was a problem hiding this comment.
It is possible to simplify this by calling next() here as well as in line 80 and removing line 76 completely, isn't it?
There was a problem hiding this comment.
peekNext is used to avoid consuming lines which are not matched by the parser. If next is used and the parser determines that the line did not match the message currently being parsed the start of another message might be lost.
Using next() everywhere would make the code easier to read / better to understand, but could lead to some unexpected behaviour in edge casses.
There was a problem hiding this comment.
You can use hasNext(String) to simplify this
src/main/java/edu/hm/hafner/analysis/parser/GhsMultiParser.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/hm/hafner/analysis/parser/GhsMultiParser.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/hm/hafner/analysis/parser/GhsMultiParser.java
Outdated
Show resolved
Hide resolved
| // This does not match the whole message, return without issue. | ||
| return Optional.empty(); | ||
| } | ||
| String currentLine = lookahead.peekNext(); |
There was a problem hiding this comment.
You can use hasNext(String) to simplify this
|
@uhafner thanks for the feedback. |
Updated two Parsers to use the LookaheadParser class instead of a deprecated base class.
Both Tickets are handled in one pull request to avoid updating the Versioning twice for similar changes.