-
Notifications
You must be signed in to change notification settings - Fork 185
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
Adding WaffleInfo class #23
Conversation
Regarding |
|
||
// Add Version Information as attributes | ||
String version = WaffleInfo.class.getPackage().getImplementationVersion(); | ||
if(version!=null) { |
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 am going to nitpick, but the rest of the code tries to use a convention where there's a space after if
and between !=
, so if (version != null)
.
I only have one problem with this implementation: it wraps a lot of calls in a try/catch exception. None of this code is expected to fail (the domain one can be checked as I mentioned above). So why try to wrap the exception within the info class at all (makes sense in the I'd be happy to merge without that, but of course I'd like to hear the counterarguments. |
This also needs an update to CHANGELOG, please. |
I'll clean some things up and add a note to the CHANGELOG later tonight...
I originally wrote this while trying to debug problems that did throw exceptions and wanted to be able to collect the exceptions and ask a reasonable question... with the 1.5 branch, it no longer throws an exception, but very easily could if the wrong JNA .jar files are included This is why I think it should catch the exceptions and present them in the XML rather then fail entirely. (Though I'm really fine either way) |
Lets not catch them, or at least start with that. Don't you get a stack trace if you're missing or have the wrong JAR? Maybe a demo or a diagnostics app can do the catching? |
This keeps one try/catch around looupAccount since that is expected to throw an exception if it does not exist
Hopefully everything looks good now -- let me know if anything else should change |
IWindowsAuthProvider auth = new WindowsAuthProviderImpl(); | ||
Element node = doc.createElement("lookup"); | ||
node.setAttribute("name", lookup ); | ||
try { |
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.
Did you mean to leave this one here?
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.
Yes -- this is expected to throw an error when the name you pass in is not found. There is even a test that makes sure it throws (and catches) an exception when you lookup an unknown name
I merged this, thank you. |
Like #14, but hopefully smaller and easier to digest. This just adds the helper class and a test to exercise it. I'll post a servlet to get this info after this gets in.
What is the expected behavior when you call IWindowsAuthProvider.getDomains() when you are in a workgroup? For me this throws an exception: