material-foundation / arc-tslint

tslint linter for phabricator's arc command line tool
Apache License 2.0
7 stars 3 forks source link

Fix bin override #2

Closed mkmik closed 7 years ago

mkmik commented 7 years ago

When the user uses a "bin": "some/path" override in the .arclinter config, this extension can no longer spawn the binary to get the version by using the default binary name.

See https://secure.phabricator.com/book/phabricator/article/arcanist_lint/ in the Locating Binaries and Interpreters section and the docstring for ArcanistExternalLinter::getExecutableCommand():

Get the composed executable command, including the interpreter and binary but without flags or paths. This can be used to execute --version commands.
googlebot commented 7 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


mkmik commented 7 years ago

hey @googlebot

My employer confirms they signed the CLA. The company name is Bitnami, and I double-checked that this commit has been authored with my mkm@bitnami.com account (which is associated with my mmikulicic github account, although not as a primary email)

mkmik commented 7 years ago

I just remembered that when I last had to sign the google CLA while I was working for another company, there was a google group all employees granted contributors rights are supposed to be member of. I'll doublecheck that.

mkmik commented 7 years ago

I'm now member of the google group Bitnami used to sign the CLA

googlebot commented 7 years ago

CLAs look good, thanks!

mkmik commented 7 years ago

friendly ping

adarshaj commented 7 years ago

This fixes #1

adarshaj commented 7 years ago

Can the collaborator pls review this for a merge? I'm interested in using this.

/cc @jverkoey

jverkoey commented 7 years ago

Apologies for this taking so long to merge, I appear to have missed the original batch of notifications.

mkmik commented 7 years ago

@jverkoey thanks! would it be possible to merge it into master?