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

Projects
None yet
2 participants
@Godin
Member

Godin commented May 25, 2017

This was forgotten in #400

@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

@@ -41,9 +41,6 @@ private Java9Support() {
*/
public static byte[] readFully(final InputStream is)
throws IOException {
if (is == null) {

This comment has been minimized.

@marchof

marchof May 29, 2017

Member

@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.

@marchof

marchof May 29, 2017

Member

@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.

This comment has been minimized.

@Godin

Godin May 29, 2017

Member

@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.

@Godin

Godin May 29, 2017

Member

@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.

This comment has been minimized.

@marchof

marchof May 29, 2017

Member

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

@marchof

marchof May 29, 2017

Member

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

@marchof marchof merged commit 817d0ae into master May 29, 2017

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@marchof marchof deleted the issue-541 branch May 29, 2017

@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.