Skip to content

Add :exclude_paths to the comman line options #45

Closed
wants to merge 3 commits into from

8 participants

@juanje
juanje commented Jun 25, 2012

Add the option :exclude_paths, that already exists, to the command line options.

@acrmp
Owner
acrmp commented Jun 25, 2012

Thanks Juanje. That was fast.

Can you add some tests for the command-line as well?

@juanje
juanje commented Jun 25, 2012

Sure.

I was looking for test for other command-line options and I didn't find. I found integration test for having help for the command-line options and a specs for the methods at lib/foodcritic/command-line.rb, but not for calling the options or testing the options.
Are there any way I missed or should I just add to the spec or the features?

Thanks

@acrmp
Owner
acrmp commented Jun 25, 2012

The feature mappings for the command-line options aren't that obvious, listing them below here for reference:

-r, --[no-]repl                  Drop into a REPL for interactive rule editing.
features/interactive_console.feature

-t, --tags TAGS                  Only check against rules with the specified tags.
features/choose_rules_to_apply.feature

-f, --epic-fail TAGS             Fail the build if any of the specified tags are matched.
features/continuous_integration_support.feature

-c, --chef-version VERSION       Only check against rules valid for this version of Chef.
features/limit_rules_to_specific_versions.feature

-C, --[no-]context               Show lines matched against rather than the default summary.
features/show_lines_matched.feature

-I, --include PATH               Additional rule file path(s) to load.
features/include_custom_rules.feature

-S, --search-grammar PATH        Specify grammar to use when validating search syntax.
features/specify_search_grammar.feature

-V, --version                    Display the foodcritic version.
features/command_line_help.feature

Thanks,

Andrew.

@juanje
juanje commented Jun 25, 2012

Oh... ok.
Then I'll add a new one for this.

Thanks :-)

@juanje
juanje commented Jun 29, 2012

Hi Andrew,

I've addded the tests and also fix a bug in the exclusion of the files at files_to_process(). The glob was looking just at the passed path level, but not checking for attributes/*.rb, etc.

I'm not sure if you like the tests and the changes I proposed. Please, have a look and tell me if you preffer something in another way, so I'll get it changed.

Thanks

@acrmp
Owner
acrmp commented Jun 29, 2012

Thanks Juanje. This is looking good.

The issue you are having with exclusions is because the rake task currently assumes that you are passing the full glob in for the excluded paths.

This is fairly normal for rake tasks, but it may make less sense to pass the exclusion through as a glob on the command line as there is potential for confusion with shell globbing. What do you think?

The tests helpers you've added in juanje/foodcritic@03c78eb mean we have three copies of the logic for identifying test warnings which isn't great. Can we remove the duplication here?

@acrmp
Owner
acrmp commented Aug 27, 2012

Hi Juanje,

I realise we let this one stall. This would still be really useful - what can I do to help move this forward?

@juanje
juanje commented Aug 28, 2012

Oh, Andrew, I'm so sorry :-/

I got into some new projects and I forgot to finish this issue.

I'll have to test this code with the master. Probably I'll have to change something.
I agree about the duplication, but it felt to me that the code I resuse was WIP and I didn't want to move it. At the same time, it was out of the scope and I need it there. That's the reason to copy there.

Maybe, as you have the big picture, you can see the better place for placing the code that identify test warnings so I can use it from here without duplication.

Thanks for ping me and sorry for the delay...

@anicholson

+1 this would be great, as foodcritic is currently incorrectly picking up test directories and finding false positives. @acrmp / @juanje , what's the state of play here? Anything I can do to help?

@acrmp acrmp added a commit that referenced this pull request Jan 21, 2014
@juanje juanje Exclude paths at the command line.
Refs #45, #148, #207.
82cd0ef
@acrmp
Owner
acrmp commented Jan 21, 2014

Merged this with a couple of tweaks. Thanks again Juanje!

@docwhat
docwhat commented Jan 23, 2014

@acrmp Seems to work great! Cool!

@jperry
jperry commented Feb 6, 2014

So if I want to exclude two cookbooks I need to do --exclude cookbookA --exclude cookbookB? Is there a way to exclude both these cookbooks with only one --exclude? For example maybe it accepts a delimiter (ie. --exclude cookbookA,cookbookB)? Could this be added?

@drywheat
drywheat commented Feb 6, 2014

It doesn't look like it. The values are stored and evaluated with the assumption that you're not providing a comma-separated list.

Storage:
https://github.com/acrmp/foodcritic/blob/master/lib/foodcritic/command_line.rb#L67-L69

Processing:
https://github.com/acrmp/foodcritic/blob/82cd0efa76d7420631bc955d170a751e9fbf3e98/lib/foodcritic/linter.rb#L215-L216

@jperry
jperry commented Feb 6, 2014

yeah, I tested and it doesn't work. It's okay just wondering if that would be a nice addition?

@jaymzh
Collaborator
jaymzh commented Nov 27, 2014

Can we close this PR now, since a version of this has been merged?

@odcinek
Collaborator
odcinek commented Mar 31, 2015

Closing this since that feature was merged, see 82cd0ef.

@odcinek odcinek closed this Mar 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.