[WIP] Check git status before ejecting

[melanieseltzer]

Related Issue:

285

Summary: Checking git status before ejecting because ejecting will not work with a dirty repo. It's working as is but I'm classifying this as WIP because I wanted to get some opinions on a few things:

• @AWolf81 you mentioned adding a try/catch during eject but I wasn't clear where to add that (maybe I misread) - any thoughts after seeing this PR code?
• We needed some way to track whether the repo was clean (because the installation in the exit shouldn't run if ejecting failed due to repo being dirty). So I did this:

emitter({
  channel: uncleanRepo === true ? 'exit' : 'stderr',
  text,
  uncleanRepo,
});

Then used uncleanRepo in some if statements. Weirdly, I had to get super specific or it would always run the installation. For e.g. something like this wouldn't work:

if (uncleanRepo === true) {
 ....
} else {
 ...
}

Had to get specific with uncleanRepo === false as you can see in the code. What are people's opinions on that?

• We can also do this in the reducer to make it idle again instead of a fail status. Let me know if that would be preferable.

// If Eject task was not successful, make it idle 
if (task.name === 'eject' && !wasSuccessful) {
  nextStatus = 'idle';
}

Screenshot

[superhawk610]

I can shed some light on why the strict equality was necessary, mainly that the exit channel in taskRun is used when running any task, not just ejecting. Thus, there are 3 possible values for uncleanRepo - true, false, or undefined. You want to ignore undefined, since that means you're running a non-eject task, and since false and undefined are both falsey, you can't just use

if (!uncleanRepo) { /*...*/ }

you have to specify, as you've done:

if (uncleanRepo === false) { /*...*/ }

However, only true is truthy in this case, so

// this is overkill
if (uncleanRepo === true} { /*...*/ }

// this should work fine
if (uncleanRepo) { /*...*/ }

[melanieseltzer]

@superhawk610 Ah! That makes total sense. Thanks for the explanation 😄 I've cleaned up those truthy checks.

[AWolf81]

Oh, I missed the failing test. Please add uncleanRepo: false to saga.next() in task.saga.test line 287 this should fix the issue.

[melanieseltzer]

@AWolf81 Thanks, all fixed!

[codecov[bot]]

Codecov Report

Merging #290 into master will increase coverage by 0.05%. The diff coverage is 57.14%.

@@            Coverage Diff             @@
##           master     #290      +/-   ##
==========================================
+ Coverage   20.46%   20.52%   +0.05%     
==========================================
  Files         239      239              
  Lines        3708     3713       +5     
  Branches      380      381       +1     
==========================================
+ Hits          759      762       +3     
- Misses       2674     2676       +2     
  Partials      275      275

Impacted Files	Coverage Δ
src/reducers/tasks.reducer.js	60% <100%> (ø)	:arrow_up:
src/sagas/task.saga.js	60.68% <50%> (-0.04%)	:arrow_down: