Skip to content

Cleanup error marshaling and generating code #183

Closed
wants to merge 6 commits into from

4 participants

@tclem
libgit2 member
tclem commented Jun 15, 2012

This is my second attempt to clean up error handling a bit. Here are the highlights.

  • I don't like Class or Type so I'm calling the libgit2 error class Category instead.
  • I'm using a custom marshaler for GitError, but we still have to manually do the UTF8 marshaling for the error message
  • Error Code and Category are now available on the exception as true properties so that consumers can use them instead of parsing the error message we generate.

This PR replaces #163

@tclem tclem referenced this pull request Jun 15, 2012
Closed

Csharp friendly errors #163

@nulltoken nulltoken commented on the diff Jun 16, 2012
LibGit2Sharp/Core/GitErrorMarshaler.cs
@@ -0,0 +1,44 @@
+using System;
+using System.Runtime.InteropServices;
+
+namespace LibGit2Sharp.Core
+{
+ internal class GitErrorMarshaler : ICustomMarshaler
@nulltoken
libgit2 member
nulltoken added a note Jun 16, 2012

Classy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nulltoken nulltoken and 1 other commented on an outdated diff Jun 16, 2012
LibGit2Sharp/LibGit2SharpException.cs
namespace LibGit2Sharp
{
/// <summary>
/// The exception that is thrown when an error occurs during application execution.
/// </summary>
- [Obsolete("This type will be removed in the next release. Please use LibGit2SharpException instead.")]
- public class LibGit2Exception : LibGit2SharpException
@nulltoken
libgit2 member
nulltoken added a note Jun 16, 2012

Please do not remove the obsolete LibGit2Exception yet.This will be done after the next version (v0.10.0) is released. @dahlbyk has already pushed #181 to deal with this

@tclem
libgit2 member
tclem added a note Jun 18, 2012

Cool. Added this back in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nulltoken
libgit2 member

As discussed in #137, there are no so many error codes, and depending on the method being executed, the same error code may mean something different.

I'm not in favor of exposing those Categories and ErrorCodes as properties to the consumer as this would compel him to understand the correct meaning of "this error code in that context". I'd prefer exposing concrete Exceptions, deriving from LibGit2SharpException (RepositoryNotFoundException, ...).

My feeling is that if we're not able to infer correct exceptions from the (codes|categories|executionContext) there's zero chance that the consumer will be able to correctly handle those (codes|categories|executionContext) tuples by himself.

Moreover, by exposing our own exceptions, I think we might be more committed to maintain their "contract" (what they mean, when they're thrown) rather than if we just hand out to the consumer what libgit2 just returned.

However, there may be headaches down the road... For instance, when the Klass/Category is Invalid, instead of throwing a derived LibGit2SharpException, should we rather throw an plain ArgumentException?

Thoughts?

@dahlbyk
libgit2 member
dahlbyk commented Jun 16, 2012

👍 for ArgumentException and its derivatives for incorrect arguments, and descriptive subclasses of LibGit2SharpException instead of exposing the internal error code.

@tclem
libgit2 member
tclem commented Jun 18, 2012

Even if we do add specific exception types for the variety of class/code combinations, I personally would still like access to the real libgit2 error codes in the actual exception objects. When dealing with exceptional behavior, I've always found that the more you can pull away the abstractions and get to the real information, the better.

If we do go through and add new exception types that derive from LibGit2SharpException I would still want to know the libgit2 error codes/classes and not just in a pre-formatted string. I don't think these two approaches are mutually exclusive.

@nulltoken
libgit2 member

I would still want to know the libgit2 error codes/classes and not just in a pre-formatted string

Fair enough :) Please add them to the Exception.Data dictionary.

@tclem
libgit2 member
tclem commented Jun 18, 2012

Great idea.

@nulltoken
libgit2 member

https://github.com/nulltoken/libgit2sharp/tree/err fails on Mono.

Build log is available here

@tclem
libgit2 member
tclem commented Jun 18, 2012

https://github.com/tclem/libgit2sharp/tree/err should fix it. I don't have a mono setup to verify with right now unfortunately.

@nulltoken
libgit2 member

Unfortunately, it doesn't. See the build log.
Previous build was targeting a squashed version of your changes. In order to ease the comparing, I've unsquashed everything and pushed it @ https://github.com/nulltoken/libgit2sharp/tree/err

I don't know if the same instance of Marshaler is used to call marshalNativeToManaged() and CleanUpNativeData()

@Haacked Haacked and 2 others commented on an outdated diff Jun 19, 2012
LibGit2Sharp/LibGit2SharpException.cs
@@ -48,6 +54,16 @@ public LibGit2SharpException()
}
/// <summary>
+ /// Initializes a new instance of the <see cref = "LibGit2SharpException" /> class.
+ /// </summary>
+ public LibGit2SharpException(GitErrorCode code, GitErrorCategory category, string message) : base(message)
+ {
+ Data["libgit2.code"] = this.code = code;
+ Data["libgit2.class"] = this.category = category;
+ isLibraryError = true;
@Haacked
Haacked added a note Jun 19, 2012

Why did you stop exposing the GitErrorCode as a strongly typed property? We actually reference that in GHfW. Am I supposed to reach into Data["libgit2.code"] instead? That makes the code uglier and feels more fragile.

@nulltoken
libgit2 member
nulltoken added a note Jun 20, 2012

Why did you stop exposing the GitErrorCode as a strongly typed property?

I'm not sure to follow you. GitErrorCode has been turned Internal more than a year ago (cf 2bd0630)

@Haacked
Haacked added a note Jun 20, 2012

Ah, I think in our branch we exposed it as a property. Why would you make this internal and not give me anything else I can use to know exactly what the error is? Parsing exception messages is piss poor and likely to change. Reaching into the Data property is ugly too.

@dahlbyk
libgit2 member
dahlbyk added a note Jun 20, 2012

The advantage of Data is that it's automatically serialized, whereas custom properties are not. What about public properties that encapsulate retrieving the code/class from Data?

@Haacked
Haacked added a note Jun 20, 2012

@dahlbyk Sure. As a consumer of the API, I sort of don't care what the backing storage of the properties are. As it stands, I have my own extension methods that do this, but it seems like a no-brainer to have it in the class as part of the API.

Another option is to implement the serialization constructors for the exception type per FX Design Guidelines: http://msdn.microsoft.com/en-us/library/ms229064.aspx

@dahlbyk
libgit2 member
dahlbyk added a note Jun 21, 2012

Implementing deserialization isn't hard, it's just more effort than using Data.

@Haacked
Haacked added a note Jun 21, 2012

Sure, I really don't care how it's done. :)

But while we're on the topic, it does seem odd to me because Data is read/write, no? So that would allow any code along the stack to modify properties in there. Thus if you created GitErrorCode as a wrapper property, it wouldn't really be readonly, though semantically that's exactly what I'd expect.

All for a little less effort?

@nulltoken
libgit2 member
nulltoken added a note Jun 21, 2012

@Haacked

All for a little less effort?

It's not about avoiding implementing the deserialization. Although that's a nice side benefit :)

You can peek at the following comments for a better context

@Haacked
Haacked added a note Jun 21, 2012

@nulltoken ah, thanks for the context! I like the idea of exposing specific exception classes. But I didn't see such classes yet.

@nulltoken
libgit2 member
nulltoken added a note Jun 22, 2012

@Haacked You're right. They have to be created yet as discussed in #137.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nulltoken nulltoken commented on the diff Jun 22, 2012
LibGit2Sharp/LibGit2SharpException.cs
/// </summary>
- /// <param name = "info">The <see cref="SerializationInfo "/> that holds the serialized object data about the exception being thrown.</param>
- /// <param name = "context">The <see cref="StreamingContext"/> that contains contextual information about the source or destination.</param>
- protected LibGit2SharpException(SerializationInfo info, StreamingContext context)
- : base(info, context)
+ public GitErrorCode Code
@nulltoken
libgit2 member
nulltoken added a note Jun 22, 2012

@tclem @Haacked As previously discussed, I'm really not willing to make the GitErrorCode and GitErrorCategory types publicly exposed, nor to expose such properties. Indeed, I don't feel the grain is fine enough (yet?) to safely build a stable/long lasting error handling strategy.

Therefore, for now, I'd prefer to only expose int typed values in the Data dictionary.
This makes the libgit2 native error codes available for logging purpose.

In order to reduce the pain, we'll indeed have to add proper exceptions to LibGit2Sharp. Which one would you see fit?

@nulltoken
libgit2 member
nulltoken added a note Jun 22, 2012

@tclem @Haacked I've created #189 to cope with the lack of proper exceptions issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nulltoken nulltoken referenced this pull request Jun 22, 2012
Closed

Create proper exceptions #189

@nulltoken
libgit2 member

@tclem I manually merged it in vNext. I focused on making the native error data accessible but dropped the Marshaler for now.

I don't have a mono setup to verify with right now unfortunately.

Now that @travisbot is watching our back with a Mono build for each PR, it should be easier ;-)

❤️

@nulltoken nulltoken closed this Aug 10, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.