Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix gitDir Bug for git specific executables #162

Merged
merged 9 commits into from
May 17, 2017

Conversation

DeFuex
Copy link
Contributor

@DeFuex DeFuex commented May 8, 2017

Changelog

  • changing res.bin !== 'npm' to res.bin === 'git'

Why

It's not possible to get the getDir path for git specific executions, in underlying directory paths and doing a precommit for lint-staged. Using 'git' options specifically declares to use options that include the getDir option for doing precommits.

TODO

  • use 'git' options only

Notes


Error report
SyntaxError: Unexpected token import at transformAndBuildScript (gui/node_modules/jest-
    runtime/build/transform.js:320:12)

Closes #158

@codecov
Copy link

codecov bot commented May 8, 2017

Codecov Report

Merging #162 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #162   +/-   ##
=======================================
  Coverage   89.74%   89.74%           
=======================================
  Files           3        3           
  Lines          39       39           
  Branches        5        5           
=======================================
  Hits           35       35           
  Misses          4        4
Impacted Files Coverage Δ
src/runScript.js 75% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12e38f0...15cd436. Read the comment docs.

@okonet
Copy link
Collaborator

okonet commented May 8, 2017

Hey @DeFuex! Thanks for your contribution. Can you please add a description to your PR so we have a paper trail? Also, can you rebase your commit and include a reference to the corresponding GHI (Closes #..).

Also, can you add a code comment explaining why is this line ever needed? See #158 (comment)

Thanks!

@billyvg
Copy link

billyvg commented May 11, 2017

One issue that I'm having is that res.bin gives me an absolute path to git, so this condition will never work, perhaps we should check if res.bin ends with git?

Inverses logic of checking for "not equal to `npm`", instead check
that binary to run is equal to `git` (and accounting for absolute
paths). This means that commands besides `npm` will also run in CWD
instead of `gitDir`.

Closes lint-staged#158
Change git binary check to match end of string (for absolute paths)
Copy link
Collaborator

@okonet okonet left a comment

Choose a reason for hiding this comment

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

Does endsWith work in node 4.x?

@okonet
Copy link
Collaborator

okonet commented May 17, 2017

Can you please also add a test for this case? After that it's good to merge! Thanks again for taking your time on this one.

@okonet
Copy link
Collaborator

okonet commented May 17, 2017

There is a test already that is tests that git is called with the correct arguments. We'll need to update the test and check that other tasks aren't.

@billyvg
Copy link

billyvg commented May 17, 2017

@okonet Doesn't this https://github.com/okonet/lint-staged/blob/master/test/runScript.spec.js#L98 cover that test?

Will add a test for findBin, is there a way to add to this PR without going through @DeFuex's branch?

@okonet
Copy link
Collaborator

okonet commented May 17, 2017

Looks like you're right! I think then it can be merged. 👍

@okonet okonet merged commit c7283b7 into lint-staged:master May 17, 2017
@DeFuex
Copy link
Contributor Author

DeFuex commented May 17, 2017

sry guys, was at an event today, everythings merged 😄

@tnguyen14
Copy link

Any thought on when this change will be released?

@billyvg
Copy link

billyvg commented May 19, 2017

@tnguyen14 it's released @3.4.2

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

Successfully merging this pull request may close these issues.

4 participants