remove gitDir config option #327

camacho · 2017-11-06T20:31:21Z

close #271

codecov · 2017-11-06T20:36:03Z

Codecov Report

Merging #327 into master will increase coverage by 0.12%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #327      +/-   ##
==========================================
+ Coverage   97.15%   97.28%   +0.12%     
==========================================
  Files          10       10              
  Lines         176      184       +8     
  Branches       26       25       -1     
==========================================
+ Hits          171      179       +8     
  Misses          5        5

Impacted Files	Coverage Δ
src/runAll.js	`100% <100%> (ø)`	⬆️
src/resolveGitDir.js	`100% <100%> (ø)`	⬆️
src/runScript.js	`100% <100%> (ø)`	⬆️
src/generateTasks.js	`100% <100%> (ø)`	⬆️
src/getConfig.js	`97.29% <100%> (+0.15%)`	⬆️

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 0e40d2c...0e174ee. Read the comment docs.

okonet · 2017-11-06T21:59:24Z

Amazing work @camacho! Does tests cover all edge cases? I didn't see any tests where gitDir isn't process.cwd. Could you add some, please?

camacho · 2017-11-06T23:18:31Z

Those cases should be covered by the spec changes. I adjusted the old tests to account for a different gitDir by mocking what is returned by the resolveGitDir and process.cwd functions (instead of passing in the gitDir config option). The test checks also including passing in the cwd option to execa if the gitDir is different than process.cwd and the bin is not git

camacho · 2017-11-06T23:51:17Z

test/generateTasks.spec.js

@@ -77,8 +88,21 @@ describe('generateTasks', () => {
    })
  })

+  it('should not match non-children files', () => {
+    const relPath = path.resolve(path.join(process.cwd(), '..'))
+    resolveGitDir.mockReturnValueOnce(relPath)


@okonet check for filtering out files that aren't in the cwd but show up because gitDir is diff

camacho · 2017-11-06T23:52:13Z

test/runScript.spec.js

-  it('should pass cwd to execa if gitDir option is set for non-npm tasks', async () => {
-    const res = runScript(['test', 'git add'], ['test.js'], scripts, { gitDir: '../' })
+  it('should pass cwd to execa if gitDir is different than process.cwd for non-npm tasks', async () => {
+    resolveGitDir.mockReturnValue('../')


@okonet check for proper execa cwd if gitDir is diff than process.cwd

sudo-suhas

Looks good overall, only minor changes required.

sudo-suhas · 2017-11-07T02:54:00Z

src/generateTasks.js

    const fileList = files
+      // Only worry about children of the CWD
+      .filter(file => file.startsWith(cwd))


Let's use path-is-inside here. From the readme:

It turns out this question isn't trivial to answer using Node's built-in path APIs. A naive indexOf-based solution will fail sometimes on Windows, which is case-insensitive (see e.g. isaacs/npm#4214).

sudo-suhas · 2017-11-07T03:29:41Z

test/resolveGitDir.spec.js

@@ -13,14 +12,4 @@ describe('resolveGitDir', () => {
    expect(resolveGitDir('.')).toEqual(expected)


resolveGitDir no longer accepts an argument. Please remove the parameter '.' in the function invocation.

camacho · 2017-11-07T03:59:33Z

@sudo-suhas updated based on feedback

sudo-suhas

Thanks @camacho!

sudo-suhas · 2017-11-07T04:16:35Z

@okonet Do you think it would be a good idea to publish this with the next tag?

okonet · 2017-11-08T12:00:46Z

@sudo-suhas why not do just breaking change? I feel like I'm not having enough time to batch changes into bigger releases and I don't want to postpone on new features from PRs.

okonet · 2017-11-08T12:01:36Z

@camacho could you please resolve the conflicts and update the branch?

Also I don't quite get your comments on tests: do you mean our tests aren't exhausting or what exactly?

camacho · 2017-11-08T12:07:03Z

@okonet sorry for not being more clear. The tests seem extensive enough. I was trying to highlight the places in the code where we test the gitDir being different than the process cwd. As far as I can tell, all cases are covered, but I am happy to add more tests if you have any cases in mind that are uncovered.

conflicts resolved

sudo-suhas · 2017-11-08T12:21:49Z

@okonet Breaking changes is a good idea. That will suffice.

okonet · 2017-11-09T10:15:22Z

@camacho one question that struck me before the merge: how this PR behaves with old configs that has the option? Will it output the deprecation message?

sudo-suhas · 2017-11-09T10:18:53Z

@okonet Good catch! I think it will print the Unknown option warning. We should update getConfig.js so that it prints the deprecation warning similar to https://www.npmjs.com/package/jest-validate#deprecation.

okonet · 2017-11-09T10:21:17Z

I've tested it and it displays a generic message right now:

● Validation Warning:

  Unknown option "gitDir" with value "." was found.
  This is probably a typing mistake. Fixing it will remove this message.

Please refer to https://github.com/okonet/lint-staged#configuration for more information...

which is confusing. Let's add a proper deprecation message, please.

You'll need to add deprecatedConfig and pass it to the validation function here https://github.com/okonet/lint-staged/blob/master/src/getConfig.js#L123

See https://github.com/facebook/jest/tree/master/packages/jest-validate for more information.

okonet · 2017-11-09T10:32:43Z

@camacho here is the patch you need to apply:

diff --git a/src/getConfig.js b/src/getConfig.js
index dc93440..fe8e684 100644
--- a/src/getConfig.js
+++ b/src/getConfig.js
@@ -30,6 +30,14 @@ const defaultConfig = {
   verbose: false
 }
 
+const deprecatedConfig = {
+  gitDir: config => `  Option ${chalk.bold('gitDir')} was removed.
+
+  lint-staged now automatically resolves '.git' directory.
+
+  Please remove ${chalk.bold('gitDir')} from your configuration.`
+}
+
 /**
  * Check if the config is "simple" i.e. doesn't contains any of full config keys
  *
@@ -121,6 +129,7 @@ function validateConfig(config) {
 
   const validation = validate(config, {
     exampleConfig,
+    deprecatedConfig,
     unknown: unknownValidationReporter,
     comment:
       'Please refer to https://github.com/okonet/lint-staged#configuration for more information...'

camacho · 2017-11-10T20:06:37Z

Requested changes pushed

sudo-suhas · 2017-11-11T01:11:30Z

@camacho could you please add a snapshot test for this?

sudo-suhas

Only minor changes. Thank you @camacho for addressing all the feedback. Hopefully this is the last one 😉

sudo-suhas · 2017-11-11T05:18:07Z

test/getConfig.spec.js

@@ -211,6 +211,17 @@ describe('validateConfig', () => {
    expect(console.printHistory()).toMatchSnapshot()
  })

+  it('should not throw and should print validation warnings for old config', () => {


Can we change this test title to should print deprecation warning for gitDir option?

sudo-suhas · 2017-11-11T05:18:47Z

test/getConfig.spec.js

@@ -211,6 +211,17 @@ describe('validateConfig', () => {
    expect(console.printHistory()).toMatchSnapshot()
  })

+  it('should not throw and should print validation warnings for old config', () => {
+    const invalidMixedConfig = {


Rename variable to configWithDeprecatedOpt please.

sudo-suhas · 2017-11-11T05:21:16Z

test/__snapshots__/getConfig.spec.js.snap

+"
+WARN ● Deprecation Warning:
+
+Option gitDir was removed.


@okonet Does the formatting for this text seem a bit odd to you?

camacho · 2017-11-11T05:31:12Z

updated

okonet · 2017-11-11T08:15:57Z

Had to amend from master and force push master :(

Lessons learned: don't merge breaking changes from the bed on the iPad 🤦‍♂️

okonet · 2017-11-11T08:16:10Z

Thanks again for your work!

camacho · 2017-11-11T08:19:47Z

Happy to contribute @okonet

* Remove `gitDir` config option * Automatically resolve `.git` directory in the project Closes #271 BREAKING CHANGE: `gitDir` option deprecated and will be ignored

sudo-suhas · 2017-11-11T08:27:19Z

Had to amend from master and force push master :(

Good thing you caught it early.

remove gitDir config option and close #271

bf7dd09

cleanup tests

3673070

okonet requested a review from sudo-suhas November 6, 2017 21:58

okonet previously approved these changes Nov 6, 2017

View reviewed changes

Merge branch 'master' into patch/271

f27d20d

camacho commented Nov 6, 2017

View reviewed changes

sudo-suhas requested changes Nov 7, 2017

View reviewed changes

update based on feedback

daad31d

camacho dismissed okonet’s stale review via daad31d November 7, 2017 03:58

sudo-suhas previously approved these changes Nov 7, 2017

View reviewed changes

Merge branch 'master' into patch/271

ff34559

camacho dismissed sudo-suhas’s stale review via ff34559 November 8, 2017 12:19

okonet previously approved these changes Nov 9, 2017

View reviewed changes

okonet mentioned this pull request Nov 9, 2017

Strange behavior when working with sub directories #324

Closed

update message for gitDir

f145865

camacho dismissed okonet’s stale review via f145865 November 10, 2017 20:06

add snapshot test for gitDir config

8d9a79c

sudo-suhas requested changes Nov 11, 2017

View reviewed changes

updated based on feedback

904ed83

Minor whitespace adjustment for deprecation msg

0e174ee

sudo-suhas approved these changes Nov 11, 2017

View reviewed changes

okonet approved these changes Nov 11, 2017

View reviewed changes

okonet merged commit 32d80de into lint-staged:master Nov 11, 2017

camacho deleted the patch/271 branch November 13, 2017 18:13

dependencies bot mentioned this pull request Nov 16, 2017

Update lint-staged in / from 4.3.0 to 5.0.0 opencomponents/storage-adapters#6

Merged

This was referenced Dec 1, 2017

Update lint-staged in / from 4.3.0 to 6.0.0 JetBrains/create-react-kotlin-app#6

Merged

Update lint-staged in / from 4.2.3 to 6.0.0 JetBrains/ring-ui#19

Merged

sudo-suhas mentioned this pull request Jan 4, 2018

Working relative to package.json, not git directory #373

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove gitDir config option #327

remove gitDir config option #327

camacho commented Nov 6, 2017

codecov bot commented Nov 6, 2017 •

edited

okonet commented Nov 6, 2017

camacho commented Nov 6, 2017 •

edited

camacho Nov 6, 2017

camacho Nov 6, 2017 •

edited

sudo-suhas left a comment

sudo-suhas Nov 7, 2017

sudo-suhas Nov 7, 2017

camacho commented Nov 7, 2017

sudo-suhas left a comment

sudo-suhas commented Nov 7, 2017

okonet commented Nov 8, 2017

okonet commented Nov 8, 2017

camacho commented Nov 8, 2017 •

edited

sudo-suhas commented Nov 8, 2017

okonet commented Nov 9, 2017

sudo-suhas commented Nov 9, 2017

okonet commented Nov 9, 2017 •

edited

okonet commented Nov 9, 2017

camacho commented Nov 10, 2017

sudo-suhas commented Nov 11, 2017

sudo-suhas left a comment

sudo-suhas Nov 11, 2017

sudo-suhas Nov 11, 2017 •

edited

sudo-suhas Nov 11, 2017

camacho commented Nov 11, 2017

okonet commented Nov 11, 2017

okonet commented Nov 11, 2017

camacho commented Nov 11, 2017

sudo-suhas commented Nov 11, 2017

		@@ -13,14 +12,4 @@ describe('resolveGitDir', () => {
		expect(resolveGitDir('.')).toEqual(expected)

remove gitDir config option #327

remove gitDir config option #327

Conversation

camacho commented Nov 6, 2017

codecov bot commented Nov 6, 2017 • edited

Codecov Report

okonet commented Nov 6, 2017

camacho commented Nov 6, 2017 • edited

camacho Nov 6, 2017

Choose a reason for hiding this comment

camacho Nov 6, 2017 • edited

Choose a reason for hiding this comment

sudo-suhas left a comment

Choose a reason for hiding this comment

sudo-suhas Nov 7, 2017

Choose a reason for hiding this comment

sudo-suhas Nov 7, 2017

Choose a reason for hiding this comment

camacho commented Nov 7, 2017

sudo-suhas left a comment

Choose a reason for hiding this comment

sudo-suhas commented Nov 7, 2017

okonet commented Nov 8, 2017

okonet commented Nov 8, 2017

camacho commented Nov 8, 2017 • edited

sudo-suhas commented Nov 8, 2017

okonet commented Nov 9, 2017

sudo-suhas commented Nov 9, 2017

okonet commented Nov 9, 2017 • edited

okonet commented Nov 9, 2017

camacho commented Nov 10, 2017

sudo-suhas commented Nov 11, 2017

sudo-suhas left a comment

Choose a reason for hiding this comment

sudo-suhas Nov 11, 2017

Choose a reason for hiding this comment

sudo-suhas Nov 11, 2017 • edited

Choose a reason for hiding this comment

sudo-suhas Nov 11, 2017

Choose a reason for hiding this comment

camacho commented Nov 11, 2017

okonet commented Nov 11, 2017

okonet commented Nov 11, 2017

camacho commented Nov 11, 2017

sudo-suhas commented Nov 11, 2017

codecov bot commented Nov 6, 2017 •

edited

camacho commented Nov 6, 2017 •

edited

camacho Nov 6, 2017 •

edited

camacho commented Nov 8, 2017 •

edited

okonet commented Nov 9, 2017 •

edited

sudo-suhas Nov 11, 2017 •

edited