Want "all" arg to '-f' option. #11

Closed
jaymzh opened this Issue Feb 22, 2012 · 17 comments

Comments

Projects
None yet
2 participants
Collaborator

jaymzh commented Feb 22, 2012

I'm using foodcritic as a pre-commit hook, and I want it to fail in all cases, so rather than listing every tag which could get out of date, I want to be able to say "-f all"

(Actually, in my case what I want is "-f all,~fcXXX" but that should come as a side-effect of "all" support)

How feasible is this? Thanks.

Owner

acrmp commented Feb 22, 2012

This is both a good addition and eminently doable (the best combination).

Owner

acrmp commented Feb 22, 2012

Apparently I've already implemented this too, but you need to use any rather than all.
https://github.com/acrmp/foodcritic/blob/c230bdd6be6e89ed113f20bb11a9c6a543fa9c67/lib/foodcritic/linter.rb#L119

$ foodcritic -f any,~FCxxx foo

Can you try this and let me know if it does the trick.

Collaborator

jaymzh commented Feb 22, 2012

OK, now I'm THOROUGHLY confused.

-f any

works, but

-f any -f ~FC014

does NOT work... but yet:

-f any,~FC014

does work! But it shouldn't work, because that's an OR, and waht I want is an AND (see Issue #10). huh?

acrmp added a commit that referenced this issue Feb 25, 2012

Fix tag logic, refs #11.
This is a breaking change to tag processing which should bring us in line with
Cucumber.
Owner

acrmp commented Feb 25, 2012

Hi Phil,

I've tweaked the handling of tags at the command line so that AND and OR should match the Cucumber behaviour:
https://github.com/cucumber/cucumber/wiki/Tags

Can you pull the current HEAD and let me know if this now makes more sense.

$ git clone https://github.com/acrmp/foodcritic.git && cd foodcritic
$ bundle install
$ bundle exec rake
$ foodcritic -f any -f ~FC014 my_cookbook_path

Thanks.

Collaborator

jaymzh commented Feb 27, 2012

I did a 'git pull --rebase' on my repo, but didn't pull down anything new and the behavior is still the same. The latest change seems to be 6 days ago... am I missing something?

Collaborator

jaymzh commented Feb 27, 2012

Nevermind I didn't have the upstream defined. Rebuilding now.

Collaborator

jaymzh commented Feb 27, 2012

This works! Thanks! I can now do things like:

   -f any -f ~FC014 -f ~FC023

Now, to be extra picky... ;) ... the ability to do something like this would be really awesome:

  -f any -f '~(FC014,FC023)'

Basically "any && NOT (FC014 || FC023)" which should expand to "any && NOT FC014 && NOT FC023" I tested that, but it doesn't work.

Owner

acrmp commented Feb 27, 2012

Cool. Thanks for taking the time to test this.

This change has the potential to break existing CI setups so I'm going to bump the gem major version when this is released.

I'm not keen on extending the expression syntax outside of the Cucumber tag syntax people may be already familiar with. If people really need more complex expressions than can be expressed this way I'd accept a patch that takes an arbitrary Ruby expression.

@acrmp acrmp closed this Feb 27, 2012

Collaborator

jaymzh commented Feb 27, 2012

sounds good to me. Thanks!!

Collaborator

jaymzh commented Feb 28, 2012

Oh - do you know when you plan to do said release?

acrmp added a commit that referenced this issue Feb 28, 2012

Owner

acrmp commented Feb 28, 2012

Hi Phil,

I'm intending to release this change in a gem later in the week.

BTW - saw your pull request on the awesomeness that is FPM.
Would love to see OS packages for foodcritic - great idea!

Collaborator

jaymzh commented Feb 28, 2012

I just pulled in 0.11.0 which seems to have the right -f magic....

I have a shellscript that builds foodcritic as RPMs into the /opt/chef bundled ruby (since most of the deps are already there). It required a bit of fpm work arounds, but it all works now...

Owner

acrmp commented Feb 28, 2012

Very nice - would be great if you can share this.

I think you're going to find that 0.11.0 doesn't include the changes in e7c12ab and will only work for basic scenarios.

Looks like there is an update to 1.9.3 in the works for Omnibus too:
http://tickets.opscode.com/browse/CHEF-2871

Collaborator

jaymzh commented Feb 29, 2012

Here's the script. The comments assume a VERY new omnibus build (the move from /opt/opscode to /opt/chef).

#!/bin/bash

# This script automates the hell of building foodcritic RPMs

# This list can be generated by doing something like this on a clean system:
#
#   /opt/chef/embedded/bin/gem install foodcritic
#
# Then taking the output which will look something like this:
#  Successfully installed gherkin-2.7.7
#  Successfully installed gist-2.0.4
#  Successfully installed sexp_processor-3.0.10
#  Successfully installed ruby_parser-2.3.1
#  Successfully installed coderay-0.9.8
#  Successfully installed slop-2.1.0
#  Successfully installed method_source-0.6.7
#  Successfully installed pry-0.9.7.4
#  Successfully installed yard-0.7.5
#  Successfully installed pry-doc-0.3.0
#  Successfully installed rak-1.4
#  Successfully installed foodcritic-0.9.0
#
# And shoving the last column into this list.
package_list='gherkin-2.8.0
gist-2.0.4
sexp_processor-3.0.10
ruby_parser-2.3.1
coderay-0.9.8
slop-2.1.0
method_source-0.6.7
pry-0.9.7.4
yard-0.7.5
pry-doc-0.3.0
rak-1.4
yajl-ruby-1.0.0
foodcritic-0.11.0'

# This is either /opt/chef or /opt/opscode depending on your version of Chef
base_chef_path='/opt/chef'

# RPM namespace. Instead of these being called 'rubygem-<whatever>' and
# confusing people, we name them something obvious. For most shops
# this will be something like 'chef-gem'.
rpm_namespace='chef-gem'

# You probably don't need to change this
tmp_script="/tmp/foodcritic-build-script.$$"

function create_script() {
  cat >$tmp_script <<EOF
#!/bin/bash

# These regexes do:
#  * First replace invalid version requirements like 'foo > 4 < 5' with
#    correct ones like 'foo > 4, foo < 5' (line 1)
#  * Nuke all gems provided by Chef itself (lines 2 - n-1)
#  * Munge the rest of the requirements into our namespace
perl -pi -e 's/(rubygem-[\w]+) (>=? [\d.]+) (<=? [\d.]+),/\$1 \$2, \$1 \$3,/g;\\
             s/rubygem-chef( [<=>]+ [\d\.\w]+)?,?//g;\\
             s/rubygem-nokogiri( [<=>]+ [\d\.\w]+)?,?//g;\\
             s/rubygem-json( [<=>]+ [\d\.\w]+)?,?//g;\\
             s/rubygem-treetop( [<=>]+ [\d\.\w]+)?,?//g;\\
             s/rubygem/${rpm_namespace}/g' "\$1"

if [[ "\$1" =~ 'foodcritic' ]] ; then
  perl -pie -e "s!tar \-zxf \%SOURCE0!tar \-zxf \%SOURCE0\nsed \-i \-e 's@/usr/bin/env ruby@$base_chef_path/embedded/bin/ruby\@g' \\\\\\\$RPM_BUILD_ROOT$base_chef_path/embedded/lib/ruby/gems/1.9.1/bin/foodcritic!g" "\$1"
fi
EOF
  chmod +x $tmp_script
}

function build_rpm() {
  name=$(echo $1 | perl -pi -e 's/(.*)-[\d\.]+/$1/g')
  ver=$(echo $1 | perl -pi -e 's/.*-([\d\.]+)/$1/g')
  # Currently we use this super hacky way of pre-processing the spec file
  # by specifying our script as the editor... however, once a version of
  # fpm is released with https://github.com/jordansissel/fpm/pull/163 in it
  # we'll switch to using --post-process
  EDITOR=$tmp_script $base_chef_path/embedded/bin/fpm -s gem -t rpm -v $ver \
    -n $rpm_namespace-$name --gem-gem $base_chef_path/embedded/bin/gem \
    -e -d 'chef-full >= 0.10.4, chef-full < 0.11.0'  $name
  if [ $? -ne 0 ] ; then
    echo "Building of $1 broke, bailing out"
    exit 1
  fi
}

create_script

echo 'Checking for FPM'
$base_chef_path/embedded/bin/gem list | egrep -q '^fpm '
if [ $? -ne 0 ] ; then
  echo 'Not installed, installing'
  sudo $base_chef_path/embedded/bin/gem install fpm
fi

echo 'OK, building all of our RPMs'
cd /tmp
for i in $package_list; do
  build_rpm $i
done

rm -f $tmp_script

echo
echo "Congrats! Your rpms are in /tmp/$rpm_namespace-*.rpm"
echo
Collaborator

jaymzh commented Feb 29, 2012

And I see your point. Thanks. Can you update this Issue when 0.12 comes out? Thanks!

Owner

acrmp commented Mar 4, 2012

This has been released in 1.0.0.

Collaborator

jaymzh commented Mar 7, 2012

looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment