🚸 Warn user if they are not in a git repository

[mikelinhas]

The change introduces a check to bail out if the user runs the tool outside a git repository. It will also warn the user. This handles this common pitfall gracefully instead of logging the stdout errors that happen in these cases.

Description

Issue: #1275

Tests

• All tests passed.

Did not create a new test for this.

[carloscuesta]

Thanks for the PR! 🙌🏼

[carloscuesta]

I would probably avoid handling this through a helper and instead improve the way we show errors to users.

This way we can catch all exceptions handling them gracefully

[carloscuesta]

Suggested change

	if(!isGitRepository()) {
	return console.log(
	chalk.red(
	"\nError: Seems that you're trying to commit outside a git repository \n" +
	'Please navigate to a git repository to use `gitmoji` \n'
	)
	)
	}

[carloscuesta]

This is what's causing the error to display in a weird way, we can just remove this:

gitmoji-cli/src/utils/isHookCreated.js

Lines 16 to 19 in 68bc2a4

	} catch (error) {
	console.error(error)
	}
	}

[carloscuesta]

Then handle the exception here:

gitmoji-cli/src/commands/commit/withClient/index.js

Lines 41 to 51 in 68bc2a4

	} catch (error) {
	console.error(
	chalk.red(
	'\n',
	'Oops! An error occurred. There is likely additional logging output above.\n',
	'You can run the same commit with this command:\n'
	),
	'\t',
	error.escapedCommand
	)
	}

[carloscuesta]

We can use error.stderr, error.command & error.shortMessage to see if we can display a meaningful error, otherwise fallback to what we have

[mikelinhas]

The only issue I see is that it will not be a controlled error.
We won't know inside the catch block what has actually happened.

Another thing to note - if we don't bail out with the helper, we ask the user for input knowing they are kind of doomed to fail. 🙈

[segersniels]

That original error handling was nice, since it told you the git command it was going to execute before it failed.

That was only because it didn't exit early in the flow and it eventually failed on the last step, being git commit. So logically we need to adjust the flow or error message slightly to make sense now. Ideally we just try catch execa executions and handle each use case accordingly (eg. not all of them need to mention the command retry).

Something like this could work (rough idea):

import chalk from 'chalk'
import { ExecaError } from 'execa'

const printExecaError = (error: ExecaError, includeCommandRetry = false) => {
  let message = `${error.stderr}\n${error.shortMessage}\n\nOops! An error occurred. There is likely additional logging output above.\n`

  if (includeCommandRetry) {
    message += `You can run the same commit with this command:\n${error.escapedCommand}`
  }

  console.error(chalk.red('\n', message))
}

export default printExecaError

Another alternative would be if we custom wrapped execa which then throws custom controlled ExecutionError's that we could then use to check instanceof and treat accordingly in a general try catch. Would allow us to properly differentiate between general runtime errors and execa errors (we don't currently do that and assume that all errors are execa related).

But I'll leave the final decision up to @carloscuesta to decide the direction to go in.

[carloscuesta]

Both are fine!

I don't think that loosing the escapedCommand thingy is too much of a loss, if we improve the overall error handling

So feel free to go with any of the options!

[mikelinhas]

Great! Thanks.
I removed the initial approach and just made the error handling more generic.
Hope it helps.

[segersniels]

Thanks for making the required changes! Ideally the files you touched are also converted to TS 🙏 But don't worry if you're not up to it. I can always open another PR with those changes after this one is merged.

[mikelinhas]

I'm a bit weary of migrating to Typescript file by file.
I might be doing it wrong, but as soon as I change one file, I have to basically change all the depending files and tests in a cascading effect.
I'll keep an eye on the project and lend a hand if I figure out how you're doing it.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.72%. Comparing base (6bf779d) to head (c8f8af3).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1276      +/-   ##
==========================================
+ Coverage   91.42%   91.72%   +0.29%     
==========================================
  Files          28       28              
  Lines         280      278       -2     
  Branches       66       66              
==========================================
- Hits          256      255       -1     
+ Misses         23       22       -1     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[carloscuesta]

Thanks!!