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

allow options to invert regex and glob and combine them all #5

Merged
merged 2 commits into from
Sep 13, 2014

Conversation

rliebling
Copy link

before, you could specify a regex or glob to match, but not both.
Now you can specify both, but also a regex NOT to match and a glob
NOT to match.

In order for reflex to react to the change ALL filters must say "OK"

so, for example,

reflex -G '.*' -- echo {}

will not match any files whose names begin with '.' (eg vim swap files)

rich added 2 commits December 26, 2013 10:17
will make it easier to enhance this ability by just storing
different functions in the Reflex struct.
before, you could specify a regex or glob to match, but not both.
Now you can specify both, but also a regex NOT to match and a glob
NOT to match.

In order for reflex to react to the change ALL filters must say "OK"
@cespare
Copy link
Owner

cespare commented Dec 27, 2013

Hey @rliebling, thanks for the PR.

My gut reaction is that combining globs and regexes should be allowed, but not inversions. It just doesn't seem like it's worth adding two more flags and associated code for the feature. Can you describe some of the use cases you see for inverted matches? You gave one example of -G '.*' to not match vim swap files, but when I've been using reflex day to day I've found I generally give a positive match; say, -G '*.go'. (Also in this specific case you can formulate 'don't start with .' as a regex: -r '^[^\.]', though admittedly that's pretty clunky.)

I'm trying to keep reflex fairly small; my tagline is

Run a command when files change. Few frills.

which is already stretching the truth, but I'd like to keep that from becoming a complete lie :D

Thoughts? Feel free to try and convince me I'm wrong.

Another approach would be to do it more like find does and use some flag to indicate that the following flag is inverted (find uses !) but that requires custom flag parsing and seems pretty hideous to me.

@rliebling
Copy link
Author

@cespare - your call whether to merge or not. I'm fine either way. thanks for the nice tool.

But, i'll make my case for you to consider.

For the particular use case that motivated me to actually make the change, i want to use reflex to trigger a reindex of my fastrAck utility (https://github.com/rliebling/fastrAck). There are a number of files that should not trigger a reindex. Here's a brief list (regex's here) of what i'd like to exclude:

  1. .cindex
  2. .*.sw[mnop]$
  3. .git
  4. .log$

Yes, the first 3 could probably be merged into one case by excluding all files containing a path component starting with '.'. It's a little messy as a regex, especially as the name may start with a '.' or just have some deeper path component start with a '.' (eg p1/p2/p3/.foobar.swp). Now, to also exclude files ending in '.log' .... i'm not saying it cannot be done, just that i don't think there's a way to do it that I'd be happy writing :) And, I'm pretty sure i'd find other cases of files i want to exclude.

Not sure how common of a situation this would be. I think use of grep -v is probably very common, and that is the moral equivalent of what I've added here. And, the fact is that it's difficult to write regex's or globs that exclude patterns.

As for keeping things simple, i agree in principle, but question whether this complicates things. Yes, it adds a couple new flags. Code-wise, the flags themselves are trivial. (I understand that usage might be more complicated by having to read through more options.) For the implementation, i'd argue that my first commit (just refactoring) is a win because I think it simplifies and improves the code. There's no reason the Reflex class (er, type/struct) should have to know about globs and regex's. Just give it a func(name string) bool and it will be happy.

This also makes it easy for anyone else to extend with whatever wacky filters they might want (even if their extensions never get merged back into cespare/reflex. You might imagine reflex being used as a library by fully separating the front-end option handling etc from the actual Reflex type.)

And, although the second commit adding the inverse matching does add some complexity, and perhaps ugliness, that is restricted to the configuration. If, for example, you separated out all the configuration/option-handling from the actual 'business logic', then the complexity would just infect the file with the option configuration. And, frankly that's already where most of the ugliness lives, as it should be. That is, the ideal is to push all the ugliness to the details and have the main logic be clean and easy to read. My first commit improves that, while the second one adds a bit of ugliness where I think it's ok :)

Anyway, i won't be offended or bothered if you reject this PR. The beauty of github, and of people like you sharing quality tools, is that it's easy to fork and do as you want.

@cespare cespare merged commit c567b82 into cespare:master Sep 13, 2014
@cespare
Copy link
Owner

cespare commented Sep 13, 2014

Hey @rliebling, sorry for the long delay.

Upon giving this more thought, I like this change. It also lays a good foundation for #12 and #9.

I've merged the PR and made some follow-up adjustments:

  • 80074f4: Code style, naming, tests, and also made the mechanism even more general. Multiple -r, -g, etc flags are now allowed (so you can do something like reflex -r foo -r '\.go$' echo {}.)
  • 54d5abb: Documentation
  • 11bb6b4: Added your name to the authors list

@rliebling
Copy link
Author

@cespare Glad to see it was helpful. Looks like you made some nice improvements on it, too.

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.

2 participants