Extend the documentation with respect to the exceptions being raised by various functions

The function discover_repository apparently raises KeyError if it cannot find a repository. I think that it is not appropriate and it also is not documented. Could it be fixed, please?

[carlosmn]

Fixed how? Without knowing what you expect it to do, it's not possible to provide any kind of fix.

KeyError is an odd class, and it's a side-effect of python having different ideas for what libgit2 returns as a GIT_ENOTFOUND error. There's a boolean to pass for IO contexts, which we apparently forgot; but what do you expect it to do if not found? Raise a different exception? Do something else?

Basically it's up to you. If you are OK with the KeyError, I can live with that. But what I do care about is the documentation. Could you please document what happens if no repository is found?

[jdavid]

Python usually raises IOError with errno 2 (ENOENT) when it does not find something in the filesystem. But in other cases it raises OSError (with the same errno).

I believe that IOError and OSError are raised if the function actually tries to access a directory/file and this attempt fails. I don't know how the function works but if it doesn't try to open/read/write/remove/list a directory/file, I think that a ValueError or LookupError would be fine. Or it can return None. It depends on the implementation. It may be a combination of all of them.

Let me make the bug report broader.
Various functions/methods raise various exceptions. Sometimes, they are reasonable but just in case that you know the implementation of the functions. E.g. pygit2.init_repository raises ValueError in case of a wrong path. On the other hand, pygit2.Repository.index.add raises IOError. It would be nice to make the exceptions raised by the functions consistent, or document the implementation of the functions or document what exceptions raise which function. Some examples (not the complete list):

• pygit2.init_repository in case of an inaccessible path
• pygit2.Index.add in case of an inaccessible path
• pygit2.Index.write in case of wrong permissions
• pygit2.Repository.reset in case of wrong oid

Maybe it would be worth mentioning what happens if another process removes the repository files in the meantime as well.

[pypingou]

One possibility would be to also use custom-class errors allowing consumer to catch the high-level exception (from which all pygit2 exceptions inherit) and display the corresponding error message

[kbd]

I just started experimenting with pygit2 and pretty much the first thing you run into (just running the example code) is that discover_repository throws a KeyError if you're not in a repository.

This is unexpected for three reasons:

1. it's not documented. I went through and read the source to make sure this was expected behavior and I wasn't doing something wrong.
2. in Python you don't expect KeyError to be raised from function calls. KeyError is used for accessing a non-existent key in a dictionary.
3. The most natural thing for discover_repository to do is return None to indicate no repository found.

Throwing an exception at all for a normal case of looking-for-and-not-discovering a repository was surprising, but a KeyError was doubly surprising because there's no mapping type involved, so it gives the impression that it's an internal error that escaped. If it threw a RepositoryNotFoundError (or a GitError with an exception message) then it would be clearer that that was expected behavior.

Of course discover_repository can't be changed now without breaking clients, but documentation of the exception would be helpful.

[jdavid]

Now it returns None and it's documented.

This issue became broader than just discover_repository, any suggestions for what should raise/return the other functions?

[kbd]

Now it returns None and it's documented.

👍

any suggestions for what should raise/return the other functions?

I think it's fine if a function that creates files throws an IOError if (eg.) the filesystem is un-writeable, or a ValueError when passed something of the right type but wrong value. The problem with discover_repository's KeyError is that a key error is not what happened. Documenting the common exceptions that your function can throw is helpful.

---

The other approach is to catch and wrap everything that comes out of your library in a library-specific exception, and not just GitError (because then you lose information, or have to inspect the inner exception) but a specific subclass of GitError for the particular way something went wrong. I think that's a lot of work for not much benefit, and if you decide to go the other way later you can always make your exception inherit from the thing it's replacing so that FileSystemUnwriteableError(GitError, IOError) is still catchable with except IOError.