Use regex to detect exclusions instead of string.Contains · Pull Request #21 · kashav/fsql

ghost · 2017-05-22T00:22:51Z

I added a struct which uses a regex to determine whether or not a file should be excluded.

if .git is excluded, it excludes .git and .git/*, and will not exclude .gitignore

in general if you have a list of exclusions .git, .gitignore the generated regexp will look like:
^\.git$|^\.git/.*$|^\.gitignore$|^\.gitignore/.*$

Fixes: #5

kashav · 2017-05-22T03:31:56Z

@jonesbrianc26 Thanks for the contribution! I'll have to review/test it in more depth tomorrow, but I took a brief glance and it's looking very nice so far.

ghost · 2017-05-22T03:44:35Z

My test additions are pretty minimal, but I think this would be a perfect chance to add something like mentioned here #20

Also this regex is rather naive, so it could definitely be optimized and would probably clean up some of the logic during building it

kashav · 2017-05-22T06:37:05Z

I've actually gotten started on something for that.

The temporary directory idea that was mentioned in #20 sounds like a cool idea, but might be a little excessive for this. Go automatically ignores anything inside a testdata directory, so the idea is to fill that directory with arbitrary files/fixtures and then use those in the tests.

ghost · 2017-05-22T12:19:11Z

I'm not too familiar with go tests unfortunately, but a good mock library would he helpful, but data driven tests would definitely fit the case here I think 👍

kashav

This looks great, I love the direction.

Left some comments for things that can be improved. Let me know what you think.

kashav · 2017-05-22T17:16:23Z

+	"strings"
+)
+
+//Excluder allows us to support different methods of excluding in the future


✔️

I think we can use this same interface to detect paths we've seen and apply regex to that to check for leading/trailing slashes (this is also naively implemented with a simple map and no regex right now).

Let me know if you'd like to work on that as well (should be in a different PR).

I can make an issue for this, see if anyone picks it up, otherwise I'll gladly take care of it!

kashav · 2017-05-22T17:19:26Z

+func (r *RegexpExclude) ShouldExclude(path string) bool {
+	var b bool
+	var ok error
+	if r.regex == "" {


When no exclusion is provided, this is running every single time and nothing is being outputted (since everything matches "").

The solution here would be to track if the expression has been built or not, and if it's been built and still is empty, then this method should return false without ever running the match method.

kashav · 2017-05-22T17:20:06Z

+
+//RegexpExclude uses regular expressions to tell if a file/path should be excluded
+type RegexpExclude struct {
+	recursive  bool


Don't think this is being used anywhere? Would be a good idea to remove it (interested in hearing what it was intended for though).

Was toying with the idea of preemptively excluding paths here, but decided against that for the scope of this PR. The idea being you could pass in flags to determine the behavior of the struct

kashav · 2017-05-22T17:23:15Z

+type RegexpExclude struct {
+	recursive  bool
+	exclusions []string
+	regex      string


I think it'd be more appropriate if this was of type *regexp.Regexp.

kashav · 2017-05-22T17:24:30Z

+		}
+		regex = or(prev, curr)
+	}
+	r.regex = regex


Since r.regex is of type *regexp.Regexp now, this should read

r.regex = regexp.MustCompile(regex)

Note that this panics on invalid regex, which I think is the appropriate solution here, since it means the input was invalid/malformed.

I agree. Behavior now is to print out the same broken error message, so I agree a panic would be preferred

kashav · 2017-05-22T17:26:27Z

+		r.buildRegex()
+	}
+
+	if b, ok = regexp.MatchString(r.regex, path); not(ok) {


This can now be reduced to:

return r.regex.MatchString(path)

kashav · 2017-05-22T17:30:30Z

+
+func TestShouldExclude_ExpectAllExcluded(t *testing.T) {
+	exclusions := make([]string, 0)
+	exclusions = append(exclusions, ".git", ".gitignore")


A little bit nitpicky, but you can do the equivalent in a single line with:

exclusions := []string{".git", ".gitignore"}

kashav · 2017-05-22T17:30:46Z

+
+func TestShouldExclude_ExpectNotExcluded(t *testing.T) {
+	exclusions := make([]string, 0)
+	exclusions = append(exclusions, ".git")


Here too 😄

kashav · 2017-05-22T17:36:08Z

+	var curr string
+	for _, exclusion := range r.exclusions {
+		prev = curr
+		if strings.HasSuffix(exclusion, "/") {


I like what's happening here, but I think there's a simpler way to do it. So instead of ^.git$|^.git/.*$, we can use ^.git(/.*)?$.

You can simplify this loop by first right-trimming any / and replacing all . with \\., then wrapping the result with ^ and (/.*)?$ (I'd recommend doing this with fmt.Sprintf instead of individual functions).

I suggest creating a slice where each exclusion is put into that form, and then joining that slice with | (using strings.Join) after the loop is done.

I like his alot. Definitely simplifies the logic

ghost · 2017-05-24T04:46:29Z

@kshvmdn

Refactored a bit of the code.

I went with what you said for all of these, great catches!

kashav · 2017-05-24T06:18:22Z

+	if r.regex == nil {
+		r.buildRegex()
+	}
+	if r.regex.String() == "" {


kashav · 2017-05-24T06:21:38Z

Thanks @jonesbrianc26!

Edit: Just FYI, you'll show up as a contributor when feature/subquery gets merged into master.

* feature/subquery: Minor clean-up Use regex to detect exclusions instead of string.Contains (#21) Fix failing tests; track tokens with slice instead of linked list Add prelim. implementation of subqueries; update package structure Fix bug with missing ending identifier in parsed subqueries Replace stack implementation with Lane (oleiade/lane) Minor tokenizer refactor Add subquery tokenizer

beeceej added 7 commits May 21, 2017 17:07

add .vscode folder to gitignore

c0582a1

build excluder

9db446e

Query excluder by regexp fix build regexp

292d9ed

integrate the excluder

7c25432

add excluder tests

ee0d045

Don't swallow error on regex

5f86c60

add the ./* case to the case where the user appends '/'

e1cdfac

kashav changed the title ~~Bugfix/fix exclusions~~ Use regex to detect exclusions instead of string.Contains May 22, 2017

kashav requested changes May 22, 2017

View reviewed changes

beeceej added 4 commits May 24, 2017 00:12

make regexp type *regexp.Regex

f5ded91

refactor regexp building

8b91445

refactor regexp building

68885b8

refactor tests

74c955c

kashav reviewed May 24, 2017

View reviewed changes

Comment thread query/excluder.go

if r.regex == nil {

r.buildRegex()

}

if r.regex.String() == "" {

kashav May 24, 2017

Copy link
Copy Markdown

Owner

Choose a reason for hiding this comment

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

nice!

kashav approved these changes May 24, 2017

View reviewed changes

kashav merged commit bb9efe2 into kashav:feature/subquery May 24, 2017

kashav mentioned this pull request May 24, 2017

Excluding directories also excludes similarly-named files #5

Closed

kashav mentioned this pull request May 25, 2017

regression test? #20

Closed

Conversation

ghost commented May 22, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kashav commented May 22, 2017

Uh oh!

ghost commented May 22, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kashav commented May 22, 2017

Uh oh!

ghost commented May 22, 2017

Uh oh!

kashav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kashav May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kashav May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kashav May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented May 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kashav commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ghost commented May 22, 2017 •

edited by ghost

Loading

ghost commented May 22, 2017 •

edited by ghost

Loading

kashav May 22, 2017 •

edited

Loading

kashav May 22, 2017 •

edited

Loading

kashav May 22, 2017 •

edited

Loading

kashav commented May 24, 2017 •

edited

Loading