Cleanup error marshaling and generating code #183

tclem · 2012-06-15T21:31:48Z

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

nulltoken · 2012-06-16T11:19:52Z

LibGit2Sharp/Core/GitErrorMarshaler.cs

+
+namespace LibGit2Sharp.Core
+{
+    internal class GitErrorMarshaler : ICustomMarshaler


nulltoken · 2012-06-16T11:26:49Z

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 · 2012-06-16T15:35:39Z

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

tclem · 2012-06-18T15:53:19Z

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 · 2012-06-18T18:04:13Z

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 · 2012-06-18T18:59:09Z

Great idea.

nulltoken · 2012-06-18T22:02:44Z

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

Build log is available here

tclem · 2012-06-18T22:07:01Z

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

nulltoken · 2012-06-19T06:03:03Z

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 · 2012-06-19T21:26:32Z

LibGit2Sharp/LibGit2SharpException.cs

+        {
+            Data["libgit2.code"] = this.code =  code;
+            Data["libgit2.class"] = this.category = category;
+            isLibraryError = true;


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.

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)

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.

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?

@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

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

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?

@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

Cleanup error marshaling and generating code #183 (comment)

Cleanup error marshaling and generating code #183 (comment)

Cleanup error marshaling and generating code #183 (comment)

Cleanup error marshaling and generating code #183 (comment)

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

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

Conflicts: LibGit2Sharp/LibGit2SharpException.cs

nulltoken · 2012-06-22T08:45:33Z

LibGit2Sharp/LibGit2SharpException.cs

-        /// <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


@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?

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

nulltoken · 2012-08-10T12:29:14Z

@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 ;-)

❤️

Cleanup error marshalling, generating code

dfc5de4

tclem mentioned this pull request Jun 15, 2012

Csharp friendly errors #163

Closed

nulltoken reviewed Jun 16, 2012
View reviewed changes

LibGit2Sharp/Core/GitErrorMarshaler.cs

namespace LibGit2Sharp.Core

{

internal class GitErrorMarshaler : ICustomMarshaler

Copy link

Member

nulltoken Jun 16, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Classy!

Add libgit2exception back in

5f61f26

Don't exposes libgit2 error code/class directly

08374d7

haacked reviewed Jun 19, 2012
View reviewed changes

tclem added 3 commits June 21, 2012 10:44

Merge remote-tracking branch 'origin/vNext' into error-handling-refactor

55ee014

Conflicts: LibGit2Sharp/LibGit2SharpException.cs

Expose ErrorCode and ErrorCategory as readonly

5e28e01

Different strategy for handling null errors

a820e0e

nulltoken reviewed Jun 22, 2012
View reviewed changes

nulltoken mentioned this pull request Jun 22, 2012

Create proper exceptions #189

Closed

nulltoken closed this Aug 10, 2012

Cleanup error marshaling and generating code #183

Cleanup error marshaling and generating code #183

Uh oh!

Conversation

tclem commented Jun 15, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nulltoken commented Jun 16, 2012

Uh oh!

dahlbyk commented Jun 16, 2012

Uh oh!

tclem commented Jun 18, 2012

Uh oh!

nulltoken commented Jun 18, 2012

Uh oh!

tclem commented Jun 18, 2012

Uh oh!

nulltoken commented Jun 18, 2012

Uh oh!

tclem commented Jun 18, 2012

Uh oh!

nulltoken commented Jun 19, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nulltoken commented Aug 10, 2012

Uh oh!

Uh oh!