| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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
| Back | FazBrowse Home | New Git URL |
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality@kwwall I have a very hard time imagining how this change would break anything in terms of backwards compatibility. And it eliminates a dependency that clearly was so tenuous that I have a hard time understanding why we kept it. I approve of this change.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality@xeno6696, @reschke - Matt, I agree with you this shouldn't really break anything unless there are some bizzaro edge cases in Apache Common Collection 4's ArrayListIterator class that treats thingslike null or empty parameters in the XML file in some unexpected way. (Backwards compatibility ideally should address failure cases in an identical manager, and not just the sunny day scenarios.) I've not looked at the ArrayListIterator source code, but it's probably a safe assumption that it doesn't do anything weird in such cases. However, it would be nice to take the opportunity of of this change and an add a JUnit test that would invoke DelegatedACR.getParameters such edge cases to test it to be sure. Maybe Julian would be so kind as to contribute such a test as well.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityI can agree with testing; however, I'd like to throw out that if we're not tied to a Vector collection type, then the java 8 streams API can both consolidate the code and possibly clarify the desired behavior in edge-case scenarios:
/** * Convert an array of fully qualified class names into an array of Class objects * @param parameterClassNames * @return The Class objects found that match the specified class names provided. */ protected final Class[] getParameters(String[] parameterClassNames) { if (parameterClassNames == null) { return new Class[0]; } List<Class> classes = Arrays.stream(parameterClassNames) .filter(Objects::nonNull) .filter(el -> !el.toString().trim().isEmpty()).map(name -> getClass(name, "parameter")) .collect(Collectors.toList()); return classes.toArray(new Class[classes.size()]); }I don't know what tests currently exist, but those are the ones that I might consider in this update. I'm not sure of the complexity of stubbing the return from the getClass call either.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityApololgies, just looked at the getClass method, I believe on invalid classname in the array we would expect an IllegalArgumentException
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThe intention of the fix was to be minimal. I checked https://commons.apache.org/proper/commons-collections/apidocs/org/apache/commons/collections4/iterators/ArrayListIterator.html and it does not mention that it does unusual things will null. Empty values are not special in arrays anyway.
If there is special processing on the the way from XML to here, it happens somewhere else.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.