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

add default switch statements #507

Merged
merged 14 commits into from Nov 5, 2019

Conversation

nvanderperren
Copy link
Contributor

tries to solve #422
since it wasn't much, I also added @param, @return, @throws etc. to the Javadocs.

@codecov
Copy link

codecov bot commented Oct 22, 2019

Codecov Report

Merging #507 into integration will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@                Coverage Diff                @@
##             integration     #507      +/-   ##
=================================================
- Coverage           49.7%   49.69%   -0.02%     
  Complexity           987      987              
=================================================
  Files                 55       55              
  Lines               7748     7750       +2     
  Branches            1406     1371      -35     
=================================================
  Hits                3851     3851              
- Misses              3433     3435       +2     
  Partials             464      464
Impacted Files Coverage Δ Complexity Δ
...edu/harvard/hul/ois/jhove/handler/TextHandler.java 0.54% <0%> (-0.01%) 2 <0> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a277f8...076fd0f. Read the comment docs.

@carlwilson carlwilson self-requested a review October 22, 2019 22:34
@carlwilson carlwilson added feature New functionality to be developed P2 Medium priority issues to be scheduled in a future release labels Oct 22, 2019
@carlwilson carlwilson added this to the v1.24-m4 Release milestone Oct 22, 2019
Copy link
Member

@carlwilson carlwilson left a comment

Choose a reason for hiding this comment

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

I mostly wholeheartedly approve of these changes, there's one or two I halfheartedly approve of but recent PRs from others lead me to suspect I'm in a minority. Checking the appropriate style guidelines didn't strengthen my points ;).

Just the one comment inline, I suspect the problem is my understanding.

Finally I'll hold off this PR until a few earlier ones are merged. Many of the changes here are logical and I'd rather deal with conflicts in this branch / PR than others.

@@ -446,11 +450,7 @@ public int parse(InputStream stream, RepInfo info, int parseIndex) {
HtmlDocDesc docDesc = null;
switch (type) {
case HTML_3_2:
default:
Copy link
Member

Choose a reason for hiding this comment

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

@nvanderperren I'm missing something with this deletion? A quick explanation for the hard of understanding please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the null values are not necessary because it's not used nor checked. Only the values that are assigned in the if or switch statements are used. But I was maybe a bit too eager.

@nvanderperren
Copy link
Contributor Author

nvanderperren commented Oct 23, 2019

Thanks @carlwilson . Next time, I'll create a new branch for the lacking Javadoc documentation and a seperate PR for that issue.

@carlwilson carlwilson merged commit 5ed4ba9 into openpreserve:integration Nov 5, 2019
@nvanderperren nvanderperren deleted the issue/422 branch November 6, 2019 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality to be developed P2 Medium priority issues to be scheduled in a future release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants