Bubble up error message

[dpr-0]

Fixes #99

Re: #99 (comment)

[ezio-melotti]

A few questions/comments:

1. How does the output look like after this changes?
2. Since this except seems higher up in the call stack, can it be triggered by other failures that are not related to sha validation?
3. IIRC the goal of the sha validation is to find out if the given commit exists in the repo, so instead of saying "Error validate sha", I would say something like f"Cannot locate sha commit. Are you inside a {config['repo']} repo?" (this is assuming this error is triggered only by failed sha validation, if not it should be moved deeper in the function that checks)
4. Can you add a test for this?

[dpr-0]

Hi @ezio-melotti, thanks for reviewing my PR.

For your questions,

1. The current output looks like this:

   $ cherry_picker 
   🐍 🍒 ⛏
   Error validate sha
   The sha listed in the branch name, 7f777ed95a19224294949e1b4ce56bbffcb1fe9f, is not present in the repository

2. and 3.

   The IRC will be raised when validate_sha(self.config["check_sha"]) or self.get_state_and_verify() fail, because both may raise ValueError, so maybe saying f"Cannot locate sha commit or get state. Are you inside a {config['repo']} repo?"?

4 . Yes, the test would be like this (tested):

def test_cli_invoked_invalid_repo():
    with pytest.raises(subprocess.CalledProcessError) as exc_info:
        subprocess.check_output("cherry_picker")
    assert exc_info.value.output.decode().startswith(
        "\U0001F40D \U0001F352 \u26CF\nCannot locate sha commit or get state. Are you inside a cpython repo?"
    )

[ezio-melotti]

Regarding the error message: it looks like the original error message (at least for the invalid sha) is clear enough, so perhaps there is no need to add the f"Cannot locate sha commit or get state. Are you inside a {config['repo']} repo?". Is this true also for the "get state"? (BTW, it's not clear just by reading the proposed message what "get state" is supposed to do, why it failed, and how to fix the problem.)

Regarding the test, there is actually a test already, but only checks the exception type:

cherry-picker/cherry_picker/test_cherry_picker.py

Lines 407 to 410 in d7c3264

	def test_is_not_cpython_repo():
	# use default CPython sha to fail on this repo
	with pytest.raises(InvalidRepoException):
	CherryPicker("origin", "22a594a0047d7706537ff2ac676cdc0f1dcb329c", ["3.6"])

You can use pytest.raises to check that the error message mentions the invalid sha and (possibly separately) whatever error message is raised when getting the state fails.

[dpr-0]

Is this true also for the "get state"?

I think the original error message is clear too:

cherry-picker/cherry_picker/cherry_picker.py

Lines 675 to 686 in d7c3264

	raise ValueError(
	f"Run state cherry-picker.state={state.name} in Git config "
	"is not known.\nPerhaps it has been set by a newer "
	"version of cherry-picker. Try upgrading.\n"
	"Valid states are: "
	f'{", ".join(s.name for s in self.ALLOWED_STATES)}. '
	"If this looks suspicious, raise an issue at "
	"https://github.com/python/cherry-picker/issues/new.\n"
	"As the last resort you can reset the runtime state "
	"stored in Git config using the following command: "
	"`git config --local --remove-section cherry-picker`"
	)

If we both agree there is no need to add the summary of error message, I can just enhance existing tests.

[dpr-0]

@ezio-melotti