Skip to content

Review exceptions #830

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jdavid opened this issue Oct 20, 2018 · 3 comments
Open

Review exceptions #830

jdavid opened this issue Oct 20, 2018 · 3 comments
Labels

Comments

@jdavid
Copy link
Member

jdavid commented Oct 20, 2018

To have one exception for every GIT_EXXX all of them inheriting from GitError, use a consistent naming, e.g. GIT_EINVALIDSPEC would become GitInvalidSpecError or maybe just InvalidSpecError.

To provide backwards compatibility use multiple inheritance, e.g. InvalidSpecError would inherit from GitError and ValueError. Eventually the base class for backwards compatibility would be remove in some future version.

To settle on naming post here the full mapping of GIT_EXXX to exception, even if this can be implemented in steps.

This follows up on issues #531 and #828

@robinst
Copy link
Contributor

robinst commented Oct 20, 2018

Sounds good to me.

Eventually the base class for backwards compatibility would be remove in some future version.

There's some precedent in Python's standard library for exceptions with multiple base classes, see this post:

https://www.reddit.com/r/Python/comments/1k9ygb/multiple_inheritance_for_library_exceptions/

So maybe it's not necessarily a bad thing that needs to be removed in the future.

@jdavid
Copy link
Member Author

jdavid commented Oct 21, 2018

yes you're right, it may be a good thing

@omniproc
Copy link

omniproc commented Apr 5, 2020

While I implemented a first version there are currently some reasons that stop us from fully switching to such a system that will probably need to be tackled one-by-one.

  • Passthrough needs to be changed to GitPassthroughError to be consistent in the naming. The issue is that while I replaced all instances in PyGit2 with the new name, Passthrough might be used by users. I therefor let GitPassthroughError inherit from Passthrough and didn't delete Passthrough yet. So, Passthrough can be viewed as deprecated and some time in future should be removed. Also since I'm not sure what side effects Passthrough has on the flow logic (it is a special error, just like GIT_EUSER) I didn't yet add it to the automatic error generation, it still has to be explicitly raised by some Python code.
  • GIT_EUSER is currently used to control the flow of some wrappers. It seems the current logic that uses GIT_EUSER return codes needs to be changed in a way that it's using the new GitUserError exception. This seems to be quite hard as I yet don't fully understand the flow logic there. Some help with this would be awesome.
  • Because of that mentioned GIT_EUSER (and the related _stored_exception logic) issue and the fact that some exceptions are currently catched and never raise it's not clear if RemoteCallbacks exceptions are ignored #996 is fixed with this implementation or if the GIT_EUSER thing first needs to be fixed/replaced. The functions of interest seem to be _fill_fetch_options, _fill_push_options, _fill_prune_callbacks, _fill_connect_callbacks and all callers of those as well as the @ffi.callback wrapper.
  • The old exception handling used check_error(err, io=False) and set io=True if it wanted a ValueError to become a IOError. I'm not sure what was the reason for that but to be backwards compatible I kept it that way. Every line that previously made a call to check_error() with io=True was replaced by a special, temporary exception class GitIOError. You can find the relevant parts if you search for GitIOError.check_result(. Those parts need to be changed in a way that the actual GIT_E error is thrown and then GitIOError can be removed. Since I'm not sure what's the reason behind the explicit raise of IOError instead of other exceptions (e.g. ValueError) in first place this would need some clearification first.
  • So far I only implemented the GIT_E* codes (and their matching classes) for the errors that are currently explicitly handled. Before all the remaining codes are implemented it would probably be a good idea to discuss what C calls currently occurre and what errors they could raise, and then add the new handling logic to them as needed. I marked the not yet implemented GIT_E* variants with a TODO in errors.py.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants