-
Notifications
You must be signed in to change notification settings - Fork 3
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
Fix bejob and added md5 and script fixes #82
Conversation
…parent and caused type error.
… needs new upload.
/** | ||
* @throws RuntimeException if a value is requested that does not exist. | ||
* @param id Configuration value to return. | ||
*/ |
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.
As you already said, runtime exceptions are not the best way to go. Is there something like a ConfigurationException? If not, would it make sense to create this kind of exception?
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.
- Apparently, it is good practice to reuse official/existing core exceptions:
https://stackoverflow.com/questions/10016550/java-best-practices-when-throwing-exceptions-throwing-core-java-exceptions - There is https://docs.oracle.com/javase/7/docs/api/javax/xml/datatype/DatatypeConfigurationException.html, but the "Datatype..." does not fit. I would not use it here.
- Maybe https://docs.oracle.com/javase/7/docs/api/java/text/ParseException.html, but I would not use it, because the configuration tree is not only build by parsing.
That's what I could find. I would make a new ConfigurationException.
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.
Ok, that sounds reasonable. Leave it for now, but I'll create an issue for it.
@@ -4,13 +4,13 @@ set -e | |||
increasebuildonly=${increasebuildonly-false} | |||
|
|||
# Check for configured plugin directories. Set empty if no dirs are configured. | |||
pluginDirectories=`grep pluginDirectories ${customconfigfile}` | |||
[[ -z "$pluginDirectories" ]] && pluginDirectories="pluginDirectories=" | |||
pluginDirectories=`grepFromConfigFile pluginDirectories $customconfigfile` |
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.
Why did you remove the curlies?
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.
I changed the line and in the course of editing removed them, apparently. As they are meaningless in this context I could leave them away, but if you wish I add them again.
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.
Was only a question :D just leave it.
You were right. I forgot this pull request.