Skip to content

Conversation

@jrolfs
Copy link
Contributor

@jrolfs jrolfs commented Mar 28, 2020

What:

Fix a bug in the validate script.

Why:

The validate script currently assumes at least one script will be run, however I ran into an edge case where no scripts should be run which results in an error as concurrently is spawned without any script arguments.

The particular edge case in which I came across this issue:

  • The validate script is being spawned from the pre-commit script
  • The package in which the scripts are run has no build or flow script defined in its package.json

How:

Only spawn concurrently if more than 0 scripts are queued to run.

Checklist:

  • Documentation N/A
  • Tests
  • Ready to be merged

@jrolfs jrolfs changed the title fix: don't attempt to run validate scripts if there are none to be run Handle edge case in validate script Mar 28, 2020
@codecov
Copy link

codecov bot commented Mar 28, 2020

Codecov Report

Merging #128 into master will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
+ Coverage   87.38%   87.50%   +0.11%     
==========================================
  Files          18       18              
  Lines         325      328       +3     
  Branches       75       76       +1     
==========================================
+ Hits          284      287       +3     
  Misses         34       34              
  Partials        7        7              
Impacted Files Coverage Δ
src/scripts/validate.js 100.00% <100.00%> (ø)

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 aa634cd...e557caf. Read the comment docs.

@jrolfs jrolfs force-pushed the validate-no-scripts branch from 85df920 to e557caf Compare March 28, 2020 02:17
Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Sounds good. Thanks 👍

@kentcdodds kentcdodds merged commit b134926 into kentcdodds:master Mar 28, 2020
@kentcdodds
Copy link
Owner

🎉 This PR is included in version 5.6.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants