Add :exclude_paths to the comman line options #45

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
8 participants
@juanje
Contributor

juanje commented Jun 25, 2012

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

@acrmp

This comment has been minimized.

Show comment
Hide comment
@acrmp

acrmp Jun 25, 2012

Member

Thanks Juanje. That was fast.

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

Member

acrmp commented Jun 25, 2012

Thanks Juanje. That was fast.

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

@juanje

This comment has been minimized.

Show comment
Hide comment
@juanje

juanje Jun 25, 2012

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@acrmp

acrmp Jun 25, 2012

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@juanje

juanje Jun 25, 2012

Contributor

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

Thanks :-)

Contributor

juanje commented Jun 25, 2012

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

Thanks :-)

@juanje

This comment has been minimized.

Show comment
Hide comment
@juanje

juanje Jun 29, 2012

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@acrmp

acrmp Jun 29, 2012

Member

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?

Member

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

This comment has been minimized.

Show comment
Hide comment
@acrmp

acrmp Aug 27, 2012

Member

Hi Juanje,

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@juanje

juanje Aug 28, 2012

Contributor

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...

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@anicholson

anicholson Jun 3, 2013

+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?

+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

This comment has been minimized.

Show comment
Hide comment
@acrmp

acrmp Jan 21, 2014

Member

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

Member

acrmp commented Jan 21, 2014

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

@docwhat

This comment has been minimized.

Show comment
Hide comment
@docwhat

docwhat Jan 23, 2014

Contributor

@acrmp Seems to work great! Cool!

Contributor

docwhat commented Jan 23, 2014

@acrmp Seems to work great! Cool!

@jperry

This comment has been minimized.

Show comment
Hide comment
@jperry

jperry 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?

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?

@sonnysideup

This comment has been minimized.

Show comment
Hide comment
@sonnysideup

sonnysideup 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

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

This comment has been minimized.

Show comment
Hide comment
@jperry

jperry Feb 6, 2014

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

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

This comment has been minimized.

Show comment
Hide comment
@jaymzh

jaymzh Nov 27, 2014

Collaborator

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

Collaborator

jaymzh commented Nov 27, 2014

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

@odcinek

This comment has been minimized.

Show comment
Hide comment
@odcinek

odcinek Mar 31, 2015

Collaborator

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

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