You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/06/04 09:08:55 UTC
[GitHub] [maven-checkstyle-plugin] stevenschenk opened a new pull request #47: [MCHECKSTYLE-401] - NullPointerException in Violation class
stevenschenk opened a new pull request #47:
URL: https://github.com/apache/maven-checkstyle-plugin/pull/47
Fixes [MCHECKSTYLE-401](https://issues.apache.org/jira/browse/MCHECKSTYLE-401).
Both `file` and `line` field are nullable in the constructor. Equals method now uses `Objects.equals()` for those fields to handle the nullability.
Following this checklist to help us incorporate your
contribution quickly and easily:
- [x] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MCHECKSTYLE) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [x] Each commit in the pull request should have a meaningful subject line and body.
- [x] Format the pull request title like `[MCHECKSTYLE-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `MCHECKSTYLE-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [x] You have run the integration tests successfully (`mvn -Prun-its clean verify`).
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [x] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-checkstyle-plugin] elharo merged pull request #47: [MCHECKSTYLE-401] - NullPointerException in Violation class
Posted by GitBox <gi...@apache.org>.
elharo merged pull request #47:
URL: https://github.com/apache/maven-checkstyle-plugin/pull/47
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-checkstyle-plugin] stevenschenk commented on pull request #47: [MCHECKSTYLE-401] - NullPointerException in Violation class
Posted by GitBox <gi...@apache.org>.
stevenschenk commented on pull request #47:
URL: https://github.com/apache/maven-checkstyle-plugin/pull/47#issuecomment-863814367
> > @stevenschenk the checkstyle API states that file and line can never be null. So I expect you are fixing a problem which can never occur, only in tests.
> > I mean, how can a violation not be on a line in a file?
> > That said, I have no objections merging it anyway.
>
> My bad. Was not listed in the ticket, but I did not verify it either. I checked the constructor and there was no null check on these fields, so I assumed they were nullable. If the API states they cannot be null, should we do a `requiresNonNull()` check in the constructor?
@bmarwell So I added a `requiresNonNull()` check for `file` and `line` fields in the constructor of `Violation`, however it causes other tests to fail (For example: `CheckstyleViolationCheckMojoTesttestDefaultConfig()`. One of the test cases in that test is a violation for a missing file. When creating the violation it provides the "missing file" as file input to the constructor. As the missing file could not be resolved, resulting in a null value, it seems violation could be created with null value for `file`. This seems contradicting with the API. Is this a test case that can never occur, or could violations actually exist without a file?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-checkstyle-plugin] elharo commented on pull request #47: [MCHECKSTYLE-401] - NullPointerException in Violation class
Posted by GitBox <gi...@apache.org>.
elharo commented on pull request #47:
URL: https://github.com/apache/maven-checkstyle-plugin/pull/47#issuecomment-863978418
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-checkstyle-plugin] stevenschenk edited a comment on pull request #47: [MCHECKSTYLE-401] - NullPointerException in Violation class
Posted by GitBox <gi...@apache.org>.
stevenschenk edited a comment on pull request #47:
URL: https://github.com/apache/maven-checkstyle-plugin/pull/47#issuecomment-854548556
> @stevenschenk the checkstyle API states that file and line can never be null. So I expect you are fixing a problem which can never occur, only in tests.
> I mean, how can a violation not be on a line in a file?
>
> That said, I have no objections merging it anyway.
My bad. Was not listed in the ticket, but I did not verify it either. I checked the constructor and there was no null check on these fields, so I assumed they were nullable. If the API states they cannot be null, should we do a `requiresNonNull()` check in the constructor?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-checkstyle-plugin] elharo commented on a change in pull request #47: [MCHECKSTYLE-401] - NullPointerException in Violation class
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #47:
URL: https://github.com/apache/maven-checkstyle-plugin/pull/47#discussion_r645465377
##########
File path: src/main/java/org/apache/maven/plugins/checkstyle/Violation.java
##########
@@ -196,10 +196,10 @@ public boolean equals( Object other )
return false;
}
Violation violation = ( Violation ) other;
- return line.equals( violation.line )
+ return Objects.equals( line, violation.line )
Review comment:
changes should have unit tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-checkstyle-plugin] stevenschenk commented on pull request #47: [MCHECKSTYLE-401] - NullPointerException in Violation class
Posted by GitBox <gi...@apache.org>.
stevenschenk commented on pull request #47:
URL: https://github.com/apache/maven-checkstyle-plugin/pull/47#issuecomment-854548556
> @stevenschenk the checkstyle API states that file and line can never be null. So I expect you are fixing a problem which can never occur, only in tests.
> I mean, how can a violation not be on a line in a file?
>
> That said, I have no objections merging it anyway.
My bad. Was not listed in the ticket, but I did not verify it either. If it cannot be null, should we do a `requiresNonNull()` check in the constructor?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-checkstyle-plugin] bmarwell commented on pull request #47: [MCHECKSTYLE-401] - NullPointerException in Violation class
Posted by GitBox <gi...@apache.org>.
bmarwell commented on pull request #47:
URL: https://github.com/apache/maven-checkstyle-plugin/pull/47#issuecomment-854539904
@stevenschenk the checkstyle API states that file and line can never be null. So I expect you are fixing a problem which can never occur, only in tests.
I mean, how can a violation not be on a line in a file?
That said, I have no objections merging it anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-checkstyle-plugin] stevenschenk commented on a change in pull request #47: [MCHECKSTYLE-401] - NullPointerException in Violation class
Posted by GitBox <gi...@apache.org>.
stevenschenk commented on a change in pull request #47:
URL: https://github.com/apache/maven-checkstyle-plugin/pull/47#discussion_r645558785
##########
File path: src/main/java/org/apache/maven/plugins/checkstyle/Violation.java
##########
@@ -196,10 +196,10 @@ public boolean equals( Object other )
return false;
}
Violation violation = ( Violation ) other;
- return line.equals( violation.line )
+ return Objects.equals( line, violation.line )
Review comment:
I added a test for the equals method. Could you elaborate on what you specifically want to be tested?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-checkstyle-plugin] bmarwell commented on a change in pull request #47: [MCHECKSTYLE-401] - NullPointerException in Violation class
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #47:
URL: https://github.com/apache/maven-checkstyle-plugin/pull/47#discussion_r645832837
##########
File path: src/main/java/org/apache/maven/plugins/checkstyle/Violation.java
##########
@@ -196,10 +196,10 @@ public boolean equals( Object other )
return false;
}
Violation violation = ( Violation ) other;
- return line.equals( violation.line )
+ return Objects.equals( line, violation.line )
Review comment:
Can confirm it is running through this line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-checkstyle-plugin] stevenschenk edited a comment on pull request #47: [MCHECKSTYLE-401] - NullPointerException in Violation class
Posted by GitBox <gi...@apache.org>.
stevenschenk edited a comment on pull request #47:
URL: https://github.com/apache/maven-checkstyle-plugin/pull/47#issuecomment-863814367
> > @stevenschenk the checkstyle API states that file and line can never be null. So I expect you are fixing a problem which can never occur, only in tests.
> > I mean, how can a violation not be on a line in a file?
> > That said, I have no objections merging it anyway.
>
> My bad. Was not listed in the ticket, but I did not verify it either. I checked the constructor and there was no null check on these fields, so I assumed they were nullable. If the API states they cannot be null, should we do a `requiresNonNull()` check in the constructor?
@bmarwell So I added a `requiresNonNull()` check for `file` and `line` fields in the constructor of `Violation`, however it causes other tests to fail (For example: `CheckstyleViolationCheckMojoTest.testDefaultConfig()`. One of the test cases in that test is a violation for a missing file. When creating the violation it provides the "missing file" as file input to the constructor. As the missing file could not be resolved, resulting in a null value, it seems violation could be created with null value for `file`. This seems contradicting with the API. Is this a test case that can never occur, or could violations actually exist without a file?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org