-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Add public CategoryFilter constructor that was removed in JUnit 4.12 #1403
Add public CategoryFilter constructor that was removed in JUnit 4.12 #1403
Conversation
a1e22da
to
6ec5c8d
Compare
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.
Looks good in general, just some minor suggestions.
final Set<Class<?>> set= new HashSet<Class<?>>(); | ||
if (t != null) { | ||
Collections.addAll(set, t); | ||
private static Set<Class<?>> createSet(Class<?>[] t) { |
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.
While you're at it, can you please rename t
to something with a little more meaning?
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.
Done
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.
Done
|
||
return t.length == 1 | ||
? Collections.<Class<?>>singleton(t[0]) | ||
: new HashSet<Class<?>>(Arrays.asList(t)); |
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.
How about a LinkedHashSet
so the iteration order stays intact?
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.
The previous code used HashSet for this case. I don't think ordering matters, since these are inclusion/exclusion filters. Am I missing something?
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.
You're right: The ordering doesn't really matter. In general, I like to preserve it whenever possible so it's easier to debug/test and to not confuse users with a different order in error messages. However, in this case using HashSet
should be fine.
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.
Good point about debugging. The toString()
and describe()
methods do print out the categories. Printing them in the same order makes sense. Changed to use LinkedHashSet
in a separate commit
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.
👍
included = createSet(inclusions); | ||
excluded = createSet(exclusions); | ||
} | ||
|
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.
Is it possible to remove the duplication in these constructors by having only a single private one that does the assignments?
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 think doing that would sacrifice readability
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
This makes describe() and toString() print the categories in the same order that they were provided to the user.
6ec5c8d
to
07bd583
Compare
…unit-team#1403) * Add back public CategoryFilter constructor that was removed in JUnit 4.12. * Remove duplicated code that handles null categories. * Change CategoryFilter to use LinkedHashSet(). This makes describe() and toString() print the categories in the same order that they were provided to the user.
…unit-team#1403) * Add back public CategoryFilter constructor that was removed in JUnit 4.12. * Remove duplicated code that handles null categories. * Change CategoryFilter to use LinkedHashSet(). This makes describe() and toString() print the categories in the same order that they were provided to the user.
Note that this constructor was removed in #503