search

cespare / reflex

Run a command when files change
MIT License
3.39k stars 136 forks source link

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

Closed rliebling closed 10 years ago

rliebling commented 10 years ago

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)

cespare commented 10 years ago

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 commented 10 years ago

@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 commented 10 years ago

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:

rliebling commented 10 years ago

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