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

Update ConfigHandler.java #586

Merged
merged 7 commits into from May 6, 2020

Conversation

archivist-liz
Copy link
Contributor

@codecov
Copy link

codecov bot commented May 5, 2020

Codecov Report

Merging #586 into integration will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff               @@
##             integration     #586   +/-   ##
==============================================
  Coverage          45.63%   45.63%           
  Complexity          1046     1046           
==============================================
  Files                 57       57           
  Lines               9149     9149           
  Branches            1687     1687           
==============================================
  Hits                4175     4175           
  Misses              4424     4424           
  Partials             550      550           
Impacted Files Coverage Δ Complexity Δ
...a/edu/harvard/hul/ois/jhove/NisoImageMetadata.java 75.99% <ø> (ø) 182.00 <0.00> (ø)
.../java/edu/harvard/hul/ois/jhove/ConfigHandler.java 60.00% <100.00%> (ø) 26.00 <0.00> (ø)

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 8220f49...9dd97ee. Read the comment docs.

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.

See the inline comment, only now do I realise that I've remained true to my wavering in a single review comment, apologies. Either solution is acceptable, ping me in this issue thread or on Slack if you want to discuss anything. Thanks

@@ -371,7 +371,7 @@ public InputSource resolveEntity (String publicId, String systemId)
throws SAXException, IOException {
if (systemId.endsWith (configSchemaName)) {
try {
URL resURL = this.getClass().getResource("jhoveConfig.xsd");
URL resURL = this.getClass().getResource(configSchemaName);
Copy link
Member

Choose a reason for hiding this comment

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

+1 use of existing constant :)

/** private String constants. */
private static final String YES = "Yes";
private static final String NO = "No";
private static final String COLON = ":";
Copy link
Member

Choose a reason for hiding this comment

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

The definition of the constant COLON is a classic "matter of taste" and lacks a definitive answer in lieu of a style guide. Over the years I've taken the definitive stance of wavering on this. This Stack Exchange question states the case against. If it's to be a constant, I'd adopt a name that indicates its use, in this case, TIME_SEP perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I got the one for the Niso Image Metadata. I'll update it, even if I get personal sophomoric enjoyment from using COLON as a constant. :) Not sure if I understand what is the issue/if there is an issue with ConfigHandler.java.

@carlwilson carlwilson added this to the Hackathon tasks milestone May 6, 2020
updated constant name as requested
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'm sorry but it's good to get some GitHub review practise right? :)

/** private String constants. */
private static final String YES = "Yes";
private static final String NO = "No";
private static final String TIME_STEP = ":";
Copy link
Member

Choose a reason for hiding this comment

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

I hate to do this but I meant TIME_SEP sorry, short for time separator as that's effectively its use here, sorry I know I'm fussy, it's a general developer character failing and job requirement. :)

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.

:) nice one

@carlwilson carlwilson merged commit 8677ad0 into openpreserve:integration May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants