search

AtomLinter / linter-codeclimate

An Atom Linter plugin for the Code Climate CLI
http://github.com/codeclimate/codeclimate
MIT License
10 stars 5 forks source link

Fixes issues #58, #59, #60 #61

Closed cgalvarez closed 7 years ago

cgalvarez commented 7 years ago

Please, read those issues to know about the problems the PR solves.

cgalvarez commented 7 years ago

Now using codeclimate binary directly, instead of wrapping in a shell call. I've tested both, OS-managed (codeclimate) and full paths (like /usr/local/bin/codeclimate), in Ubuntu 16.04, and both works.

I've extracted the logic for checking if binary is not found to the function ccBinaryFound(), which is called on errors when creating the .codeclimate.yml file too.

I've also renamed and modified the function makeEngineString() to getEngineArgs() because of executing directly codeclimate requires its args to be in the form ['-f', 'json'] (providing them like ['-f json'] makes the execution return wrong output. Also extra empty args (like "", eventually returned by makeEngineString()) make the codeclimate execution to analyze the entire project folder.

cgalvarez commented 7 years ago

Is here any pending changes to approve?

Arcanemagus commented 7 years ago

Is here any pending changes to approve?

Apparently 😛 Sorry for the delay and the many reviews, as I said I can't test this locally so I'm having to re-review this fully each time trying to catch everything I can think of.

cgalvarez commented 7 years ago

All done. Here you have a preview of the new function to notify errors (handled or not): captura de pantalla de 2017-04-15 14-55-09

I've modified the function to show only the fields that exist or have been provided, and a docblock of rany doub could raise.

Apparently :stuck_out_tongue: Sorry for the delay and the many reviews, as I said I can't test this locally so I'm having to re-review this fully each time trying to catch everything I can think of.

Don't worry. I forgot to make this PR in a new branch when I forked the repo (my fault!), that's why I need it to be merged before creating new branches the right way for two new PR I've prepared.

cgalvarez commented 7 years ago

@Arcanemagus I apologize for the lapse and the grammatical error :stuck_out_tongue_winking_eye:

Arcanemagus commented 7 years ago

I don't see anything else wrong here just looking at it, any further changes that you can see needed @maxjacobson?

maxjacobson commented 7 years ago

@Arcanemagus @cgalvarez thanks for working on this, including such a thorough review. The code LGTM, and if you've done local QA then I'm satisfied by that. I'm happy to do local QA myself if you'd like, but I'm actually not sure how to do that (it would be great to have those instructions in the README).