beyondgrep / ack2

**ack 2 is no longer being maintained. ack 3 is the latest version.**
https://github.com/beyondgrep/ack3/
Other
1.48k stars 138 forks source link

remove --[no]filter option #502

Closed rdpate closed 9 years ago

rdpate commented 9 years ago

I don't expect you to pull this as-is for two reasons. 1) It's blocking on a feature I added to File::Next which you may want to rework. 2) I couldn't figure out how to add tests for it.

This would close #499 and #500. If my File::Next pull-request is applied and this PR updated to that version of File::Next, it would also close #501.

petdance commented 9 years ago

I don't understand why we would want to remove the --[no]filter option.

rdpate commented 9 years ago

It's not needed.

If you want --filter, you have to specify it on the command line, so just specify "-" instead -- once #501 is fixed, this will work.

If you want --nofilter, you have to specify it on the command line, so just redirect stdin from /dev/null instead.

If you don't specify either --filter or --nofilter, this patch makes ack use given names (e.g. ack pattern given names) if they are specified (fixing #500), otherwise if no such names are specified, it tests -p STDIN and either uses "." or stdin.

petdance commented 9 years ago

I think I'd like to keep the explicit option available. We've had so many problems over the years with filter vs. non-filter mode, and some OSes detecting things differently, that I want to make a completely unambiguous way to say "this is/isn't filter".

rdpate commented 9 years ago

Aren't "-" and "</dev/null" completely unambiguous? (I left unambiguous instructions on stderr for anyone who tries to use it, since I didn't see a better way to deprecate the option -- but if there is a better deprecation method, it should be used.)

Though that could be a reason to keep --nofilter, if you're worried about cross-platform stdin redirection. Unix and Windows both support it, but I don't know about things like VMS. (Windows uses a magic filename, so "</dev/null" becomes "<nul". This should be made clear in the die().)

petdance commented 9 years ago

They are not as explicit as having an option that says "Do/don't run in filter mode". I also don't know about the Windows side of things.

rdpate commented 9 years ago

We could munge @ARGV and ignore --filter (still requires fixing #501). For --nofilter, we could reopen STDIN from the null device. This is better deprecation than dying on those options. How does my commit just now look?

Can we write a testcase that fails with the above behavior?

petdance commented 9 years ago

I don't see a problem with --filter and --nofilter. I don't want to get rid of them.

rdpate commented 9 years ago

Your call, of course. The other bug reports should be useful.

petdance commented 9 years ago

Yes, they're all useful. You've put a lot of work in the last couple days. Thank you.