-
Notifications
You must be signed in to change notification settings - Fork 78
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
Change method scope to support extending module classes #465
Conversation
Codecov Report
@@ Coverage Diff @@
## integration #465 +/- ##
==============================================
Coverage 49.36% 49.36%
Complexity 969 969
==============================================
Files 55 55
Lines 7666 7666
Branches 1392 1392
==============================================
Hits 3784 3784
Misses 3422 3422
Partials 460 460
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My feeling is that the 4 methods (encodeContent, encodeValue, isPropertyEmpty, cleanURIString) are really utility functions. It would be easier for maintenance to put them all in a Utility class, so we know others may use them and we don't mess with them.
*/ | ||
private boolean isPropertyEmpty (Property property, PropertyArity arity) { | ||
public boolean isPropertyEmpty (Property property, PropertyArity arity) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this method be static also ?
* @param uri | ||
* @return | ||
*/ | ||
public String cleanURIString (String uri) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this method be static ?
I'd not thought that clearly about it before @tledoux but I think you're right. Will pr a refactor. |
Included in #538 |
Portico is working to upgrade to 1.23 - we currently use a modified older version of JHOVE. There are several method/property scope changes would that allow us to continue to use some local module extensions without having to maintain a customized version of JHOVE. If they are benign (or even useful) for other users, it would be great if we could merge these.
Question for reviewer(s): one of the changes is in the
xml-hul
module. Should I increment the version? and if so to a SNAPSHOT version?Thanks!