Skip to content

Add git init and precommit hook for eslint#47

Merged
jouderianjr merged 3 commits intoifactory-solutions:masterfrom
ceejtron:commit-lint
Oct 13, 2017
Merged

Add git init and precommit hook for eslint#47
jouderianjr merged 3 commits intoifactory-solutions:masterfrom
ceejtron:commit-lint

Conversation

@ceejtron
Copy link
Copy Markdown
Contributor

As per #43 this adds precommit hooks for eslint. In addition, this adds a new question for initializing a git repository since precommit hooks can't be automatically set up unless a git repository has already been initialized.

@italopessoa
Copy link
Copy Markdown
Member

Well done @ceejtron. Seems that is everything ok. Let's wait for the reviewers. Thank you for your contribution ❤️

Comment thread hooks/eslint/index.js

const addEslintFileSuccess = (cwd, folderName) => {
const command = `npm i eslint eslint-config-airbnb babel-eslint --save-dev`
const eslintDependencies = [
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.

Nice Refactor mate

Copy link
Copy Markdown
Contributor

@jouderianjr jouderianjr left a comment

Choose a reason for hiding this comment

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

Hey, nice work! I'm pretty happy to see a new hook 💃

Comment thread hooks/git/index.js Outdated
stdio:'inherit',
}

return spawn(command, [], terminalOpts)
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.

So, We tried to find if has some issue on create-react-app related with that and I've found this PR facebook/create-react-app#1288

So, looks like that will be added at some moment, what do you think about test if already has a git started? maybe just test if exists .git folder.

@paulonotz0r
Copy link
Copy Markdown
Contributor

Nice, @ceejtron! This is really cool!

In addition, can you add commit lint too? You can use the same as used on ifactory-solutions/inside-client#8.

Thank you for the contribution.

@ceejtron
Copy link
Copy Markdown
Contributor Author

Added a check for .git as well as a commitlint hook

Copy link
Copy Markdown
Contributor

@paulonotz0r paulonotz0r left a comment

Choose a reason for hiding this comment

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

Great!

Copy link
Copy Markdown
Contributor

@jouderianjr jouderianjr left a comment

Choose a reason for hiding this comment

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

Just 2 minor things, then you will merge it! 💃

Comment thread hooks/git/index.js Outdated
console.log('\n\n')
return Promise.resolve(true)
}
else {
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.

Could you please use else in the same line? 😄

Comment thread hooks/commitlint/index.js Outdated

return spawn(command, [], terminalOpts)
})
.then(() => {
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.

You can do that as one line
.then(() => loadPackageJsonFromPath(${cwd}/${folderName}/package.json))

@ceejtron
Copy link
Copy Markdown
Contributor Author

ceejtron commented Oct 13, 2017

@jouderianjr I pushed the changes you asked for

@jouderianjr jouderianjr merged commit 1281883 into ifactory-solutions:master Oct 13, 2017
@jouderianjr
Copy link
Copy Markdown
Contributor

Awesome @ceejtron thanks for your work 💃

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