Prints error message when not in git repository

[JBizarri]

What does this PR do? Prints the default git message when not in a git repository: fatal: not a git repository (or any of the parent directories): .git

Why are we doing this? To solve issue #79 and to make it more user-friendly.

Testing performed

• Ran the toxcommand and also the pre-commit tests. All passed ✅
• Ran the git plan --version command which is now printing the default git error.
• Ran the gp --version which was already printing the default message.

Known issues

• Had to add sys.exit() because the git plan --version command was still printing the version even if not inside a git repository.
• I couldn't figure it out why the gp --version was not raising the RuntimeError exeception.

[rorybyrne]

Hey, thanks for the PR!

Maybe it would be easier to catch the NotAGitRepository exception here. You'd have to move the Settings.load() (and probably the other lines too) into the try block, and then you can add a new except underneath.

You don't have to worry about sys.exit() then.

[rorybyrne]

Had to add sys.exit() because the git plan --version command was still printing the version even if not inside a git repository.

I think we do want to allow --version to run outside of git repositories. But that will be difficult to achieve, because this exception is raised before launch_cli() is called here.

It's fine if this PR doesn't fix that, since it's kind of an architectural problem, but if you do have any ideas then it would be a welcome addition!

[JBizarri]

You'd have to move the Settings.load() (and probably the other lines too) into the try block, and then you can add a new except underneath.

I tried that (maybe I'm missing something), but if I raise the exception from the git_plan/conf.py then I can't return the Settings class.

Example:

@staticmethod
def load() -> "Settings":
    """Load the settings"""
    settings = Settings._default()
    local_settings = Settings._from_local()
    project_settings = Settings._from_project()

    local_settings.update(project_settings)  # Roll project into local settings
    settings.update(local_settings)  # Roll both into the default settings

    cwd = Path.cwd()
    settings["project_root"] = get_repository_root(cwd)

    return Settings(settings)

Will raise NotAGitRepository and settings from __cli__.py will be None and we can't make the app. And if we catch the exception from git_plan/conf.py then we can't catch from __cli__.py

I think we do want to allow --version to run outside of git repositories.

Yeah it makes senses, I didn't think that.

But that will be difficult to achieve, because this exception is raised before launch_cli() is called here.

It's fine if this PR doesn't fix that, since it's kind of an architectural problem, but if you do have any ideas then it would be a welcome addition!

I thought of some things but I think it would be just spaghetti code. And some big change like that would be out of the scope of this PR I think.

[rorybyrne]

I think we can let get_repository_root() raise its exception, and then catch it in __cli__.py. Working from your example above, this is what I had in mind:

def main():
    """Entrypoint"""
    try:
        settings = Settings.load()  # <----- inside the try block, will raise exception

        app = Application()
        app.config.from_dict(settings)
        app.wire(modules=[sys.modules[__name__]])

        args = sys.argv[1:]  # Might be []
        launch_cli(args)
    except ProjectNotInitialized:
        print("Git plan is not initialized.\n\tPlease run `git plan init`")
    except NotAGitRepository:  # <----- new except block to catch the exception
        print("Git plan can only be used in a git repository.")
    except GitPlanException as exc:
        print("git plan encountered an error.")
        print("Please open an issue at https://github.com/synek/git-plan and let us know.\n")
        print(f"The error message was '{str(exc)}'")

This means that the app won't launch at all if you are outside a git repository, but that is already what happens anyway. What do you think?

[JBizarri]

This means that the app won't launch at all if you are outside a git repository, but that is already what happens anyway.

I think it makes sense, since it won't launch anyway at least we get a more user-friendly error message. And then at a later point we'll have to deal with the version not being printed when using this solution.

At the time I think this is the best solution.

[rorybyrne]

Awesome, I can get this merged immediately if this approach works.

By the way I made an issue for the follow-on fix - https://github.com/synek/git-plan/issues/81

[rorybyrne]

One small thing before I can merge this: can you improve the error message? It's a little unfriendly (e.g. "fatal: " sounds technical).

My suggestion: "You are not in a git repository (no .git/ directory found)."

[JBizarri]

Yeah no problem. I thought since its a git tool to improve workflow the user would be familiar with the message, but I think you're right fatal seems a little too strong for something that is fixed just by running git init

[rorybyrne]

As a principle I'd like this tool to be user friendly, with a thoughtful UX. However, "hiding" git from the user completely is probably a bad idea. Maybe we can support a -v/-vv/-vvv type syntax for verbose output that includes more technical messages. Poetry does that, for example.

Anyway, thanks for fixing this! I'm going to merge it in now. Welcome aboard!