Skip to content

Integrate Ataru.#540

Merged
chastell merged 1 commit into
developfrom
feature/integrate-ataru
Jun 22, 2015
Merged

Integrate Ataru.#540
chastell merged 1 commit into
developfrom
feature/integrate-ataru

Conversation

@troessner
Copy link
Copy Markdown
Owner

Fixes #472

I really like ataru but it has quite some annoying limitations.
E.g. there is not silent flag. Which is why at the end of a rake run you'll see:

# Running:

..string -- 9 warnings:
  Dirty has no descriptive comment (IrresponsibleModule)
  Dirty#awful has 4 parameters (LongParameterList)
  Dirty#awful has approx 6 statements (TooManyStatements)
  Dirty#awful has boolean parameter 'log' (BooleanParameter)
  Dirty#awful has the parameter name 'x' (UncommunicativeParameterName)
  Dirty#awful has the variable name 'w' (UncommunicativeVariableName)
  Dirty#awful has unused parameter 'log' (UnusedParameters)
  Dirty#awful has unused parameter 'offset' (UnusedParameters)
  Dirty#awful has unused parameter 'y' (UnusedParameters)
.```

Annoying, but ok for me.

@troessner troessner force-pushed the feature/integrate-ataru branch from 82c2e73 to 896c1d4 Compare June 15, 2015 15:08
Comment thread ataru_setup.rb Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this file (and, if so, these contents)? It seems to be logically quite empty.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I removed it.

@chastell
Copy link
Copy Markdown
Collaborator

Given that we delegate the Rake task via a system call to bundle exec ataru check, can we redirect its output to not see the 9 warnings (but still see any custom ataru output)?

@bf4
Copy link
Copy Markdown
Contributor

bf4 commented Jun 15, 2015

fwiw, I wrote something like this in metric_fu, though it just checks the exit status but at least has no dependencies. I know Avdi Grimm uses xmpfilter via rcodetools in some projects

@troessner troessner force-pushed the feature/integrate-ataru branch from 896c1d4 to 4c49b6d Compare June 15, 2015 20:12
@troessner
Copy link
Copy Markdown
Owner Author

Given that we delegate the Rake task via a system call to bundle exec ataru check, can we redirect its output to not see the 9 warnings (but still see any custom ataru output)?

Grmppffff, i just hit a major roadblock with the integration.
Two things about ataru that I did not know before:

1.) ataru does not set any exit status depending on what the check actually resulted in.
Meaning that this

 echo $?
0

will always be the same. Which makes it not suitable for CI integration.

2.) When something goes wrong, e.g. a syntax error in the source code ataru still prints on STDOUT. Not on STDERR. So there is no way of telling the difference between output that one can ignore and legitimate failures.

I'll open up on issue at ataru to discuss this there.

@troessner
Copy link
Copy Markdown
Owner Author

Discussion: CodePadawans/ataru#81
Last commit is 9 months old so we probably shouldn't get our hopes up.
Any ideas how to solve this in another way?

@mvz
Copy link
Copy Markdown
Collaborator

mvz commented Jun 16, 2015

The exit status is correct if you run ataru check directly from the command line:

> ataru check
[...]
3 runs, 0 assertions, 0 failures, 1 errors, 0 skips
> echo $?
1

I think using system to run ataru may be the problem. yaks uses a different way of integrating ataru that may work better for us. See their rake task.

@troessner troessner force-pushed the feature/integrate-ataru branch from 4c49b6d to 6f570eb Compare June 16, 2015 16:14
@troessner
Copy link
Copy Markdown
Owner Author

I think using system to run ataru may be the problem.

Good catch!
I solved it differently now via

system('bundle exec ataru check') || exit(1)

which seems to work out fine - please re-review.

@troessner
Copy link
Copy Markdown
Owner Author

Regarding the output on STDOUT, let me put back this pull request into "wip" mode, I'll look into how yaks is doing that.

@troessner troessner changed the title Integrate Ataru. [WIP] Integrate Ataru. Jun 17, 2015
@chastell
Copy link
Copy Markdown
Collaborator

You can either go with Open3.capture3 or Open3.popen3:

require 'open3'

stdout, stderr, status = Open3.capture3('bundle exec ataru check')
# stdout and stderr are Strings

# or

Open3.popen3('bundle exec ataru check') do |stdin, stdout, stderr, wait_thread|
  # stdin, stdout, stderr are IO, wait_thread.value is status
end

@troessner troessner force-pushed the feature/integrate-ataru branch from 6f570eb to 453369d Compare June 21, 2015 17:29
Comment thread tasks/ataru.rake Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you want Bundle.with_clean_env like how Rails does it??

      def bundle_command(command)
        say_status :run, "bundle #{command}"

        # We are going to shell out rather than invoking Bundler::CLI.new(command)
        # because `rails new` loads the Thor gem and on the other hand bundler uses
        # its own vendored Thor, which could be a different version. Running both
        # things in the same process is a recipe for a night with paracetamol.
        #
        # We use backticks and #print here instead of vanilla #system because it
        # is easier to silence stdout in the existing test suite this way. The
        # end-user gets the bundler commands called anyway, so no big deal.
        #
        # We unset temporary bundler variables to load proper bundler and Gemfile.
        #
        # Thanks to James Tucker for the Gem tricks involved in this call.
        _bundle_command = Gem.bin_path('bundler', 'bundle')

        require 'bundler'
        Bundler.with_clean_env do
          output = `"#{Gem.ruby}" "#{_bundle_command}" #{command}`
          print output unless options[:quiet]
        end
      end
bundle_command "exec ataru check"

(and maybe see #540 (comment) ? )

@troessner troessner force-pushed the feature/integrate-ataru branch from cd91b78 to f80b14a Compare June 21, 2015 19:33
@troessner
Copy link
Copy Markdown
Owner Author

@bf4 && @chastell thanks for your tips! ( @chastell didn't know Open3 until now)

However I decided to go down a different road with the integration and just run ataru on travis, not locally when running rake. The reason for this is that with ataru we are checking our docs, or rather: our public API. So unlike specs, rubocop and what not I rarely expect this to fail, so I'd rather not make it part of our bread & butter rake run (which takes quite a while already) and just have it run on CI.

PR is updated, please re-review.

@troessner troessner changed the title [WIP] Integrate Ataru. Integrate Ataru. Jun 21, 2015
@bf4
Copy link
Copy Markdown
Contributor

bf4 commented Jun 21, 2015

I haven't run it locally, but LGTM

@chastell
Copy link
Copy Markdown
Collaborator

👍 on running Ataru on Travis only (and it passes for me locally unless I change the examples, so LGTM and merging).

chastell added a commit that referenced this pull request Jun 22, 2015
@chastell chastell merged commit 7d446ff into develop Jun 22, 2015
@chastell chastell deleted the feature/integrate-ataru branch June 22, 2015 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants