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

ci: simplified commitlint script #151

Merged
merged 28 commits into from
Nov 24, 2017
Merged

Conversation

marionebl
Copy link
Contributor

@marionebl marionebl commented Nov 22, 2017

  • Introduce a new package @commitlint/travis-cli
  • Expose new command @commitlint/travis-cli

ToDo

  • git stash before git checkout
  • git stash afterwards
  • Check if unshallow is needed

Description

  • Aborts noisily when invoked in non-Travis environment
  • Infers commit range to lint automatically from Travis Ci env variables
  • Will lint --from $TRAVIS_BRACH --to $TRAVIS_COMMIT
  • Ensures required git range is available

Motivation and Context

  • Setting up commit linting on Travis CI is too hard at the moment
    *@commitlint/travis-cli is designed to be zero-config on TravisCI
  • Motivated by Reusable Travis CI setup #99

Usage examples

{
  "devDependencies": {
    "@commitlint/travis-cli": "5.0.1"
  }
}
script:
  - commitlint-travis

How Has This Been Tested?

  • unit tests
  • dogfeeding

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

main().catch(err => {
console.log({err});
setTimeout(() => {
console.log({err});

Choose a reason for hiding this comment

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

I don't understand this - why log the error twice? why not use console.error(err); process.exit(1); instead?

});

function main() {
return new Promise((resolve, reject) => {

Choose a reason for hiding this comment

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

execa returns a Promise so you shouldn't need to use the Promise constructor

},
"scripts": {
"deps": "dep-check",
"lint": "npx xo",

Choose a reason for hiding this comment

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

why npx?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The over-simplified answer is: I introduced npx at one point to ensure the dev setup works regardless of the current state it is in. I'll keep them in travis-cli for now to be consistent, will remove them from all packages in one go in the near future.

@marionebl
Copy link
Contributor Author

Thanks for the review @felixfbecker!

@marionebl marionebl changed the title ci: simplified commitlint script WIP: ci: simplified commitlint script Nov 23, 2017
@marionebl marionebl changed the title WIP: ci: simplified commitlint script ci: simplified commitlint script Nov 24, 2017
@marionebl marionebl merged commit 3e6e6a8 into master Nov 24, 2017
@marionebl marionebl deleted the ci/simplified-commitlint-script branch November 24, 2017 15:11
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.

2 participants