fix: emptyControlStatement in DefaultModelInterpolator
#2373
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DefaultModelInterpolator Rule:EmptyControlStatement - Empty if statement.DefaultModelInterpolator Rule:EmptyControlStatement - Empty if statement
DefaultModelInterpolator Rule:EmptyControlStatement - Empty if statementDefaultModelInterpolator Rule:EmptyControlStatement - Empty if statement
9b863a7 to
fed3b09
Compare
Pankraz76
commented
May 21, 2025
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelInterpolator.java
Show resolved
Hide resolved
DefaultModelInterpolator Rule:EmptyControlStatement - Empty if statementDefaultModelInterpolator Rule:EmptyControlStatement - Empty if statement
fed3b09 to
4cef735
Compare
Pankraz76
commented
May 21, 2025
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelInterpolator.java
Outdated
Show resolved
Hide resolved
DefaultModelInterpolator Rule:EmptyControlStatement - Empty if statementDefaultModelInterpolator PMD-Rule: EmptyControlStatement
elharo
reviewed
May 22, 2025
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelInterpolator.java
Outdated
Show resolved
Hide resolved
4cef735 to
99ff878
Compare
99ff878 to
7f0c819
Compare
Contributor
Author
|
issue: 
seems persistent on windows. |
elharo
reviewed
May 25, 2025
impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelInterpolatorTest.java
Outdated
Show resolved
Hide resolved
7f0c819 to
88f6192
Compare
Contributor
Author
hopefully the real impl. works now. |
Contributor
Author
|
still: Error: DefaultModelInterpolatorTest.testProjectPropertyExtraction:619 expected: </test/path> but was: <D:\test\path> |
Contributor
Author
|
might try https://stackoverflow.com/questions/23410738/run-unit-tests-only-on-windows |
0cdc48c to
13b1981
Compare
Pankraz76
commented
May 26, 2025
impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelInterpolatorTest.java
Show resolved
Hide resolved
elharo
requested changes
May 26, 2025
impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelInterpolatorTest.java
Outdated
Show resolved
Hide resolved
impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelInterpolatorTest.java
Outdated
Show resolved
Hide resolved
impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelInterpolatorTest.java
Show resolved
Hide resolved
13b1981 to
f0ec7f8
Compare
elharo
reviewed
May 27, 2025
impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelInterpolatorTest.java
Outdated
Show resolved
Hide resolved
| String result = (String) postProcess.invoke(interpolator, projectDir, request, nonUrlExpression, nonUrlValue); | ||
|
|
||
| // Verify normalization was NOT called | ||
| verify(mockUrlNormalizer, never()).normalize(nonUrlValue); |
Contributor
There was a problem hiding this comment.
Why do we care whether the normalize method was called? This appears to test implementation. I can imagine that the normalize method would be called and that would recognize that it doesn't have to change anything. That would not be wrong.
Contributor
Author
There was a problem hiding this comment.
idk if its any good, but its seems to be reflection of current coding, therefore should be frozen with test, to be changed afterwards.
9 tasks
DefaultModelInterpolator PMD-Rule: EmptyControlStatementemptyControlStatement in DefaultModelInterpolator
9d0a319 to
2b7607a
Compare
Pankraz76
commented
Jun 6, 2025
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelInterpolator.java
Show resolved
Hide resolved
Pankraz76
commented
Jun 6, 2025
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelInterpolator.java
Show resolved
Hide resolved
Pankraz76
commented
Jun 6, 2025
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelInterpolator.java
Show resolved
Hide resolved
elharo
requested changes
Jun 6, 2025
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelInterpolator.java
Outdated
Show resolved
Hide resolved
impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelInterpolatorTest.java
Outdated
Show resolved
Hide resolved
Pankraz76
commented
Jun 6, 2025
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelInterpolator.java
Show resolved
Hide resolved
b8727f2 to
658b4e5
Compare
Pankraz76
commented
Jun 6, 2025
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelInterpolator.java
Outdated
Show resolved
Hide resolved
658b4e5 to
862198a
Compare
Pankraz76
commented
Jun 6, 2025
| .next() | ||
| .getDirectory(); | ||
| assertEquals(model.getBuild().getSourceDirectory(), sourceDirectory); | ||
| assertCollectorState(0, 0, 0, collector); |
Contributor
Author
There was a problem hiding this comment.
this is now actually second assertion. Why does not change after test ramp-up?
Pankraz76
commented
Jun 6, 2025
| @Test | ||
| public void shouldInterpolateSourceDirectoryReferencedFromResourceDirectoryCorrectly() throws Exception { | ||
| final SimpleProblemCollector collector = new SimpleProblemCollector(); | ||
| assertCollectorState(0, 0, 0, collector); |
Contributor
Author
There was a problem hiding this comment.
init state empty okay, thats fine.
elharo
requested changes
Jun 6, 2025
Contributor
elharo
left a comment
There was a problem hiding this comment.
Again, there''s just too much going on here to understand what's changing. Most of it doesn't seem to have anything to do with the PR title. In fact, I can't even find where that is fixed, or tell if it is. If you don't send more focused PRs they aren't going to be reviewed or merged.
Contributor
Author
|
yes im sorry, extracted again. Its too much going on. |
862198a to
4e6ce50
Compare
Contributor
Author
|
focus restored. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this looks like bug, or unused code. test is green meaning blindspot, better use or loose.
https://checkstyle.sourceforge.io/checks/coding/emptystatement.html
tc;