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 support for negation of -k --known-types options #448

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

packy
Copy link

@packy packy commented Apr 15, 2014

Implementation of feature request in issue #293.

@hoelzro
Copy link
Collaborator

hoelzro commented Apr 15, 2014

Awesome work @packy! This looks mostly good to me, only a few issues:

  • Could you rebase your work on top of current dev and get rid of the merge commit?
  • I think we've discusssed this before, but is --no-known-types the best name for the switch? Maybe --not-only-known-types, but that might be needlessly verbose...
  • In your patch you blow away $opt->{filters}; have you seen any issues combining --no-known-types with --type=perl, --notype=perl, and such? Combining with --type=perl seems silly, but --notype=perl might make some sense.

@petdance
Copy link
Collaborator

I think it needs to follow the convention of --no-xxxx and be --no-known-types.

@packy
Copy link
Author

packy commented Apr 15, 2014

It was rebased on top of current dev, but I figured out how to get rid of the merge commit.

And I took the name --no-known-types from Andy's suggestion in issue #293.

I hadn't tried combining it with --type=noperl, so I just did:

  • ack -K --type=noperl foo searches for 'foo' in all files except perl files
  • ack --type=noperl -K foo searches for 'foo' in all files

Which is pretty much what I'd expect it to do: negate all type filtering that had been specified up to that point.

Remember, this is mostly a way for users to turn off a --known-types that was set in their .ackrc on a case-by-case basis.

@hoelzro
Copy link
Collaborator

hoelzro commented Apr 15, 2014

@packy Right, I just wanted to make sure that the behavior followed expectations. =) Everything looks good to me; should I merge @petdance?

@petdance
Copy link
Collaborator

On Apr 15, 2014, at 2:02 PM, Rob Hoelz [email protected] wrote:

@packy Right, I just wanted to make sure that the behavior followed expectations. =) Everything looks good to me; should I merge @petdance?

I haven't even looked at it yet. See comment from earlier this morning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants