Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Method Analyzer.analyzeClass(InputStream, String) as others should provide context information when unable to read input during analysis #541

Merged
merged 1 commit into from
May 29, 2017

Conversation

Godin
Copy link
Member

@Godin Godin commented May 25, 2017

This was forgotten in #400

@Godin Godin added component: core type: bug 🐛 Something isn't working labels May 25, 2017
@Godin Godin added this to the 0.8.0 milestone May 25, 2017
@Godin Godin self-assigned this May 25, 2017
@Godin Godin requested a review from marchof May 25, 2017 21:13
@@ -41,9 +41,6 @@ private Java9Support() {
*/
public static byte[] readFully(final InputStream is)
throws IOException {
if (is == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Godin Why do we need this check specifically here? I think all other APIs like analyzeAll() will result in a NPE for a null InputStream. Which looks ok for me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marchof I guess that you misread change: it doesn't add check, but removes it exactly because we don't need it - indeed in all places for a null InputStream we throw NullPointerException instead of IllegalArgumentException, also this condition wasn't covered by tests, so I took opportunity to remove it in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Godin Sure, my bad. You actually fixed the issue - thanks!!!

@marchof marchof merged commit 817d0ae into master May 29, 2017
@marchof marchof deleted the issue-541 branch May 29, 2017 14:43
@jacoco jacoco locked as resolved and limited conversation to collaborators Jan 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: core type: bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants