Skip to content
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 better wildcard file search support #3962

Conversation

david-poindexter
Copy link
Contributor

@david-poindexter david-poindexter commented Aug 5, 2020

Add better wildcard search support

  • Style correction for Item Name sort dropdown
  • Corrected wildcard regex logic for SearchFiles

@david-poindexter david-poindexter self-assigned this Aug 5, 2020
@valadas valadas changed the title Add better wildcard search support Add better wildcard file search support Aug 5, 2020
Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, worked on this with @david-poindexter and it's tested and works much better.
Basically there was wrong escapes and "starts-with" and "ends-with" regex symbols that made no sense in a search context where we need a "contains" behaviour.

Now this does fix the search and let's call that a step 1. But in our findings, this is not awesome for performance in sites with lots of recursive files. Right now the search is enforced as being recursive and the results are filtered in code by a Regex, nothing new, it was like that already.

For performance on large installs, this has 2 problems:
1: Recursive search should be an option and maybe not the default, the code is in place but we would need some little piece of UI to turn the search to actually be recursive...
2: Getting all the files and filtering them in code using a Regex is debatable, I guess the whole results are cached before but well it may very well be that a new file search stored procedure could have better performance ?

At any rate, this is an improvement over what we currently have, just opening a can of worms here for future improvements :)

@valadas valadas added this to the Future: Minor milestone Aug 5, 2020
@@ -1845,11 +1845,11 @@ private static Regex WildcardToRegex(string pattern)
{
if (!pattern.Contains("*") && !pattern.Contains("?"))
{
pattern = "^" + pattern + ".*$";
pattern = ".*" + Regex.Escape(pattern) + ".*";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't have the start and end tokens, you wouldn't need the .* pattern, either, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I too am curious on this one, the "old way" was requires full string match, starting with the pattern at the beginning. For example

TestF as an input

Would match TestF, TestFile, TestFilters, TestF Test, etc

This new pattern would match those, but also would match

MyTestF, etc.

Is this a desired behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I too am curious on this one, the "old way" was requires full string match, starting with the pattern at the beginning. For example

TestF as an input

Would match TestF, TestFile, TestFilters, TestF Test, etc

This new pattern would match those, but also would match

MyTestF, etc.

Is this a desired behavior?

Yes, this is the desired behavior. It is a much more powerful wildcard search.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't have the start and end tokens, you wouldn't need the .* pattern, either, right?

I believe you would in order to achieve the "contains" search results.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regular Expression matching is a "contains" match by default, here's an example:
LINQPad screenshot

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this was merged already, so I'll do another PR to change this. Thanks @bdukes

@sleupold
Copy link
Contributor

sleupold commented Aug 6, 2020

I agree, left search is most common and I don't see a need for supporting search in the middle of a word.

@david-poindexter
Copy link
Contributor Author

Just to be clear, this PR is in support of the new Resource Manager. I should have made that clearer in my initial comments. Notice the PR is against the feature branch. ;-)

@mitchelsellers mitchelsellers merged commit c717cac into dnnsoftware:feature/resource-manager Aug 6, 2020
@valadas valadas modified the milestones: Future: Minor, 9.8.0 Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants