Repository navigation
Cache RegEx in ValueMatchFilter - #4056
Conversation
stevenaw
left a comment
stevenaw
left a comment
There was a problem hiding this comment.
Nicely done, thanks @manfred-brands !
Very minor nitpick suggestion related to pre-initializing the and/or filters, but it's also fine to merge as-is too. Thanks!
| case "filter": | ||
| case "and": | ||
| List<TestFilter> childFilters = new List<TestFilter>(); | ||
| var childFilters = new List<TestFilter>(node.ChildNodes.Count); |
There was a problem hiding this comment.
Nice touch to pre-initialize this. Thanks!
nitpick: Since we have to convert it to an array a few lines below, would it make sense to make this an array here instead of a list to save an array copy operation? Similar suggest for the "or" below.
There was a problem hiding this comment.
I agree with your nitpick, I should have looked how the list was used after.
Changed code to create array, extracted to a function so I only needed one copy.
|
@stevenaw Can you look at |
Thanks for noticing this 😊 I agree, it looks unused and appears to have been that way since a beta of nunit 3. That predates my involvement in the project, however, so I'd appreciate a second opinion. @CharliePoole any concerns with removing the code? |
|
@stevenaw The commenting out was part of a fairly large change I made back in 2015. The idea was that we were going to re-visit the question of how we wanted top-level selected tests (those actually selected by the user) and My 2 cents: If you find that Explicit is now working the way you want it to work (and/or as it's documented to work) I'd remove the property and all the commented out code that references it, possibly in a separate PR (but that's up to you guys). |
|
@stevenaw I will create a new issue and PR once this one is in to remove the TopLevel property. |
|
Sounds good, thanks @CharliePoole and @manfred-brands 👍 |
|
LGTM. Thanks @manfred-brands ! |
4 participants
Fixes #4055