NickCraver / StackExchange.Exceptional

Error handler used for the Stack Exchange network
https://nickcraver.com/StackExchange.Exceptional/
Apache License 2.0
860 stars 170 forks source link

Exceptional - AspNet.Core Support #89

Closed StefanKert closed 6 years ago

StefanKert commented 7 years ago

This PR should serve as a base for the further development and migration to support AspNet.Core. I´ve included the following new Projects:

Both of the Libs are built on the new AspNet Core 2 Preview, but if needed I guess we can go back to 1.2.

Currently, the Sample uses the MemoryErrorStore since we need to perform further steps to get the configuration part working.

There are also some errors but I wanted to add this PR asap so that we can start a discussion on the further direction of the changes.

There are a few things that I wanted to mention and that need more discussion:

The biggest part for me here is the Configuration. If someone has further points that need to be discussed I will add them to the list.

(Documentation for the Sourcecode and the Sample needs to be added too)

NickCraver commented 7 years ago

I appreciate the thoughts! There appears to be a lot of duplication in here based on the ASP.NET handler (totally understandable), but the ASP.NET Core version will have to differ quite a bit. For settings: yes probably JSON deserialization directly to a type and an after-load copy to base settings. Maybe. Or they may have to inherit...we have to figure out the CustomData bits first.

For handling Middleware isn't the only option, there's also diagnostic listeners as well, so I intend to bake both in.

I want to get 2.x finished everywhere else before starting ASP.NET Core, otherwise every commit is just fixing twice as many things. Once I get there (lots left with categories, commands, etc.) I'll do an initial push of ASP.NET Core to master and lay out the intended patterns there for minimal overhead. For example I don't think a handler makes sense, I think an easy-to-add-route for people makes much more sense and we'll have easy instructions on that. Separate Middleware with separate auth from a practical standpoint is always more convoluted and duplicative.

Also: need to consider the .NET Core non-ASP.NET use case, which may mean a different library layout. Or not, unsure and need to play.

I'd especially appreciate thoughts on how to share settings yet have a GetCustomData with different primitives (note: we don't need to depending on the giant meta package here...we only need a few primitives from ASP overall - I'm always against huge dependency trees).

StefanKert commented 7 years ago

Gotcha.

Thanks for the input. I will look what I can do to move some code duplication. Only wanted to get this out and get early feedback on it so that I don´t go into the wrong direction.

So basically the question is if we should put more effort in this PR or wait for the other features to be completed.

Totally agree with the dependency problem. If we could move most of the parts out of the Package to Shared we could fix this problem and also use it for non-ASP.NET I guess. In fact, the only features that need to be used from ASP.Net or need to be special for ASP.NET are the middleware/diagnostics listener aspect. The other things could be moved to the shared package. I will give this a try.

StefanKert commented 7 years ago

Just one addition:

Does it make sense to use both error handling and error viewing in a single library? For example, the console app logs your errors to a database, but it doesn´t have the ability to show those errors. There is maybe another application that should be used to show errors. Just a random thought.

This would get rid of a lot of dependencies that are needed at the moment.

NickCraver commented 7 years ago

So basically the question is if we should put more effort in this PR or wait for the other features to be completed.

At this point, I'd wait for the other base to be in...I've intentionally waited on ASP.NET Core until getting the base bits in because of all the shifts (I may still rename many methods yet for the breaking release, which won't affect consumers much but will affect new providers).

In fact, the only features that need to be used from ASP.Net or need to be special for ASP.NET are the middleware/diagnostics listener aspect.

Not a safe assumption about the listener! This is expanding a bit (I'm helping there), and will likely be generally useful. I may have a .NetCore lib used by .AspNetCore...haven't decided yet.

Does it make sense to use both error handling and error viewing in a single library?

Yeah, I see no reason to split these and have a whole other set if libs. You just use the pieces you want. Opserver will be using many exposed methods here for its dashboard as well, so the dashboard case is in mind :)

StefanKert commented 7 years ago

At this point, I'd wait for the other base to be in...I've intentionally waited on ASP.NET Core until getting the base bits in because of all the shifts (I may still rename many methods yet for the breaking release, which won't affect consumers much but will affect new providers).

Ok. If you´ve implemented your changes and have reached a state that fits our needs I will take a look if a merge of to this PR makes sense or if I start over again.

Not a safe assumption about the listener! This is expanding a bit (I'm helping there), and will likely be generally useful. I may have a .NetCore lib used by .AspNetCore...haven't decided yet.

I think there is no need for a separate .NetCore library if we split up to a Logging and a Dashboard project.

Yeah, I see no reason to split these and have a whole other set if libs. You just use the pieces you want. Opserver will be using many exposed methods here for its dashboard as well, so the dashboard case is in mind :)

Ok, so there should be further discussion on this. Should we create an Issue for this? I think a new issue would be the better place to get other opinions on this. For now, I would say that it would be cool to move for example the logging aspect to the shared project. For example, we could create a project StackExchange.Exceptional.Logging, that is based on .Net Standard and contains all the logging aspects and has a reference to StackExchange.Exceptional.Shared. This project would also be a good place for the GetCustomData, but we need to get rid of the reference to HttpContext there. I will take a look if I can come up with an example on how I imagine this lib to work because I have something particular already in mind that should fit most of our needs.

The second library would be the dashboard aspect that will depend on either AspNet.Core or System.Web. I don´t know if we should create a separate dashboard project or just use the actual library as a full project that contains dashboard and logging. I don´t know if this is right but I think that most of the non AspNet Core libs will only rely on the logging aspect. I hope this is right? Please correct me if I am wrong.

Greatly appreciate any feedback on this, I hope we can get some people in to discuss on this.

NickCraver commented 7 years ago

I think there is no need for a separate .NetCore library if we split up to a Logging and a Dashboard project.

This isn't correct, since the primitives are different between the ASP.NET Stacks. I have absolutely no desire to make a separate Dashboard project...that just makes no sense to me. We need a fork on functionality between primitives on ASP.NET and ASP.NET Core to have both logging (extracting relevant data from the primitives from logging) and display (route handling, middleware, etc.). You can't move the logging entirely to the shared project because of the dependency fork on the stacks' primitives.

This project would also be a good place for the GetCustomData, but we need to get rid of the reference to HttpContext there

...if I could do that we'd be solved in a heartbeat, that's the really easy and also far less useful solution :) I don't want to go that route unless absolutely necessary, there are some setting splits I'm going to play with first, since libs may need to append their own settings for any other reason later. This is a first-of-many type scenario to solve instead of a one-off to cripple to get by.

Overall, the final package layout planned is simply this (dependency point of view):

The question is whether a .NetCore library makes sense in the middle on the last 2, and that depends on the dependency list for primitives. For example including only 1-2 dependencies for the primitive abstractions makes sense, but if we have to bring the whole meta package stack over (effectively), that's an uncool dependency list for a simple console app that just want to log something.

Those are in a bit of flux, that's why I haven't gotten to figuring out the path there yet, but will after the base functionality changes. The honest assessment is: almost everything in this PR will be different, so it's not on the merge path, but I do want to get to these bits as soon as I can and commit them in to take ideas on the ASP.NET Core bits.

CptRobby commented 7 years ago

Excuse me @NickCraver for butting in here. I was going to add support earlier for the idea of separating the reporting / dashboard part out of Shared to simplify dependencies, but after reading what you wrote, scratching my head, and then looking at what is actually going on with those parts in Shared and the dependencies, I realized that you were right and nothing would be simplified in that respect. The only pieces of the reporting part that are in Shared are the pages, which don't actually do anything other than append strings to a StringBuilder, so no extra dependencies there. The actual HttpHandler / HttpModule stuff that actually uses them is in the ASP.NET component. So the most important separation has actually already been done. :smile:

I still have two questions though. First, I noticed that the static ErrorStore.LogException function is no longer present, but is replaced with extension methods in StackExchange.Exceptional (ASP.NET). It looks like if I were writing a console app and only wanted to use StackExchange.Exceptional.Shared, I would have to either call new Error(ex, "My Console Application").LogToStore(ErrorStore.Default); (which doesn't honor the IgnoreSettings, for example) in each of my catch blocks, or write my own version of the extension methods in ErrorExtensions.cs.

Could we not at least include a simple extension method in Shared that would just handle logging the basic stuff and honoring all of the settings. You could even give it a different name to avoid any confusion and call it LogBasic or something. It could be as simple as the following:

public static Error LogBasic(this Exception ex, bool? appendFullStackTrace = null, bool rollupPerServer = false, Dictionary<string, string> customData = null, string applicationName = null)
{
    if (!Settings.IsLoggingEnabled) return null;
    try
    {
        var settings = Settings.Current;
        if (settings.Ignore.Regexes?.Any(re => re.IsMatch(ex.ToString())) == true)
            return null;
        if (settings.Ignore.Types?.Any(type => ex.GetType().IsDescendentOf(type)) == true)
            return null;

        var error = new Error(ex, applicationName)
        {
            RollupPerServer = rollupPerServer, //This could be left out since it's geared more towards a hosting setup than a console app, but I've left it here for completeness
            CustomData = customData ?? new Dictionary<string, string>()
        };

        if (settings.GetIPAddress != null)
        {
            //This could be left out since it's geared more towards a hosting setup than a console app, but I've left it here for completeness
            try
            {
                error.IPAddress = settings.GetIPAddress();
            }
            catch (Exception gipe)
            {
                // if there was an error getting the IP, log it so we can display such in the view...and not fail to log the original error
                error.CustomData.Add(Constants.CustomDataErrorKey, "Fetching IP Address: " + gipe);
            }
        }

        if (appendFullStackTrace ?? settings.AppendFullStackTraces)
        {
            var frames = new StackTrace(fNeedFileInfo: true).GetFrames();
            if (frames?.Length > 2)
                error.Detail += "\n\nFull Trace:\n\n" + string.Join("", frames.Skip(2));
            error.ErrorHash = error.GetHash();
        }

        var logged = error.LogToStore(ErrorStore.Default);
        return logged ? error : null;
    }
    catch (Exception e)
    {
        Trace.WriteLine(e);
        return null;
    }
}

Also note that, if the rollupPerServer and GetIPAddress portions were both left in, you could just omit the LogWithoutHttpContext method entirely.

My second question is in regard to what you said about the primitives being different between ASP.NET and ASP.NET Core. What did you mean by "primitives" and how are they different? I'm still not very familiar with Core (I sing for joy when I can work on projects that are using C#6 and Framework version 4.5!), but a Google search didn't give any apparently meaningful results either (mostly SO questions about using Dependency Injection in ASP.NET Core and requests to add primitive drawing types to corefx). Just trying to get a better grasp of the issues so that I can be more helpful. :wink:

NickCraver commented 7 years ago

@CptRobby I'm not against a method in shared necessarily, but there's an overlap problem it creates. For example for all other use cases you'd now have 3 methods exposed (basic basically being a duplicate). In any case, .Shared isn't meant to be use directly so I'm not that concerned about the direct usage case. When I get both libs setup it'd likely the ignored bits will move to a central place as well.

I think what may end up happening isLogWithoutContext() moves to the base and takes a setter Action<Exception, T> for the context that the other call. I'll make a note to try this layout and see if it makes sense.

By primitives, I'm referring to things like HttpContext which are completely different between System.Web and ASP.NET Core. That's the main reason for the move to extension methods.

StefanKert commented 7 years ago

@CptRobby same here. After the thing, you wrote Nick it's clear to me that the direction I was heading you regarding the splitting to dashboard logging makes no sense to me at all.

Also, I was thinking about the move of the Loggingextension method to shared, but since you Nick wrote, that you have no plans to make Shared referenceable as a single library I think that one is also out.

Speaking of the Action<Exception, T> I was thinking of a similar solution that maybe would fit our needs, but I´ve got no time to implement it this weekend.

@NickCraver does it make sense to move some of the logging aspects to a separate project. I am thinking to taking it more from a Filler perspective. There is a filler for the different plattforms that is setting the properties for the error object and is called within the specfic LogMethod.

This would result in a method similar to this:

public static Error Log(this Exception ex, HttpContext context, bool? appendFullStackTrace = null, bool rollupPerServer = false, Dictionary<string, string> customData = null, string applicationName = null)
{
        if (!Settings.IsLoggingEnabled) return null;
        try
        {
                ConfigSettings.LoadSettings();
                var settings = Settings.Current;
                var error = new Error(ex, applicationName)
                {
                    RollupPerServer = rollupPerServer,
                    CustomData = customData ?? new Dictionary<string, string>()
                };
                if (settings.Ignore.Regexes?.Any(re => re.IsMatch(ex.ToString())) == true)
                    return null;
                if (settings.Ignore.Types?.Any(type => ex.GetType().IsDescendentOf(type)) == true)
                    return null;

                AspNetFiller.Fill(error, context); //AspNetCoreFiller.Fill(error, context);
                SharedFiller.Fill(error, appendFullStackTrace, rollupServer);

                var logged = error.LogToStore(ErrorStore.Default);
                return logged ? error : null;
        }
        catch (Exception e)
        {
                Trace.WriteLine(e);
                return null;
       }
}

This is just a simplified version. If we are using another handler for this fillers or if we implemented another way would be a good point for discussion, but I think we can share a lot of code between the "platforms" and there should also be a path for adding additional Fillers.

The question is whether a .NetCore library makes sense in the middle on the last 2, and that depends on the dependency list for primitives. For example including only 1-2 dependencies for the primitive abstractions makes sense, but if we have to bring the whole meta package stack over (effectively), that's an uncool dependency list for a simple console app that just want to log something.

Just to be on the same path -> when speaking of non-ASPNet you mean simple consoleapplications? If that's right I think we should introduce a separate project for .NetCore, because of the reasons you mentioned. Maybe we can create a Filler here too. Regarding the sharing between the .NetCore and the AspNetCore I think there is one big part that could be shared and thats the configuration. The other parts (MiddleWare, Handler) or not needed in console apps (correct me if I am wrong) and we only need a Errorhandling method. So this would result in splitting up the current ErrorExtensions class to two classes. One with the HttpContext Log method and the LogWithoutContext method should be moved over to .NetCore

The honest assessment is: almost everything in this PR will be different, so it's not on the merge path, but I do want to get to these bits as soon as I can and commit them in to take ideas on the ASP.NET Core bits.

I totally agree with this. I didn´t know that you are still changing that much bits and if you tell me that it doesn´t make sense that we continue on this right now, I would wait with this PR until you get the functionalities that are needed to be implemented and then continue with the move to AspNetCore. If you´d like to you could close this PR, but I think since we got a good discussion here, maybe we can continue and use this for the future migration.

NickCraver commented 7 years ago

@NickCraver does it make sense to move some of the logging aspects to a separate project.

Nope. We already have .Exceptional and will have .Exceptional.AspNetCore, there's just no reason to add any more dependencies, restores, redirects, etc. for some perceived split when none need exist.

I've already started on sharing the logging logic (pushed it for clarity this morning). I need to figure out GetCustomData though, that's the crux of the issue and determines where the rest goes...I just need time to play there on settings that are sensible to the user.

StefanKert commented 7 years ago

I guess I didn't state my point right. By separate I meant to move thise things to the Shared project. A new project of course makes no sense at all! Since your moving around the bits now I will take a look at it if you're ready and see if I can contribute something.

NickCraver commented 7 years ago

@StefanKert checkout the logging methods, I'll play with settings next but you can see where I've started to DRY things up a bit for what will be across projects.

NickCraver commented 7 years ago

Current progress:

public static Error Log(
    this Exception ex,
    HttpContext context,
    bool? appendFullStackTrace = null,
    bool rollupPerServer = false,
    Dictionary<string, string> customData = null,
    string applicationName = null)
{
    if (!Settings.IsLoggingEnabled)
    {
        return null;
    }
    try
    {
        // Legacy settings load (deserializes Web.config if needed)
        ConfigSettings.LoadSettings();

        // If we should be ignoring this exception, skip it entirely.
        if (ex.ShouldBeIgnored(Settings.Current))
        {
            return null;
        }

        // Create the error itself, populating CustomData with what was passed-in.
        var error = new Error(ex, applicationName, appendFullStackTrace, rollupPerServer);
        // Get any custom data from the context
        error.SetCustomData(customData, context, TODOShittyExperienceForTheUser.GetCustomData);
        // Get everything from the HttpContext
        error.SetProperties(context);
        // Set the IP Address from the settings function
        error.SetIPAddress(Settings.Current);

        if (error.LogToStore(ErrorStore.Default))
        {
            return error;
        }
    }
    catch (Exception e)
    {
        Trace.WriteLine(e);
    }
    return null;
}

As an example, the extension could be a brief as:

public static Error Log(
    this Exception ex,
    HttpContext context,
    bool? appendFullStackTrace = null,
    bool rollupPerServer = false,
    Dictionary<string, string> customData = null,
    string applicationName = null)
{
    if (Settings.IsLoggingEnabled)
    {
        try
        {
            // If we should be ignoring this exception, skip it entirely.
            if (!ex.ShouldBeIgnored(Settings.Current))
            {
                var error = new Error(ex, applicationName, appendFullStackTrace, rollupPerServer)
                            .SetCustomData(customData, context, TODOShittyExperienceForTheUser.GetCustomData)
                            .SetProperties(context)
                            .SetIPAddress(Settings.Current);
                if (error.LogToStore(ErrorStore.Default))
                {
                    return error;
                }
            }
        }
        catch (Exception e)
        {
            Trace.WriteLine(e);
        }
    }
    return null;
}

...figuring out settings next, that'll determine the no-context logger situation. That one could be shared if it doesn't call GetCustomData, but that's an unintuitive/surprising tradeoff to make, so I'm uneasy about such a thing.

NickCraver commented 7 years ago

Here's the current version for the ASP.NET library. Had to say screw it and remove HttpContext from GetCustomData to proceed. May end up having to re-evaluate that later.

public static Error Log(
    this Exception ex,
    HttpContext context,
    bool? appendFullStackTrace = null,
    bool rollupPerServer = false,
    Dictionary<string, string> customData = null,
    string applicationName = null)
{
    if (Settings.IsLoggingEnabled)
    {
        try
        {
            // Legacy settings load (deserializes Web.config if needed)
            ConfigSettings.LoadSettings();

            // If we should be ignoring this exception, skip it entirely.
            if (!ex.ShouldBeIgnored(Settings.Current))
            {
                // Create the error itself, populating CustomData with what was passed-in.
                var error = new Error(ex, applicationName, appendFullStackTrace, rollupPerServer, customData);
                // Get everything from the HttpContext
                error.SetProperties(context);

                if (error.LogToStore(ErrorStore.Default))
                {
                    return error;
                }
            }
        }
        catch (Exception e)
        {
            Trace.WriteLine(e);
        }
    }
    return null;
}

I'll see what else gets shared in a Async version, since LogToStoreAsync() would be the difference there.

StefanKert commented 6 years ago

Took a look at the new logging methods. Really like them. I think we talked past each other in this case, because I had something similar in my mind. Sharing things in ctor Error seems like a good idea to me because it instantiates all the shared properties immediately. That´s really cool!

I also like the GetCustomData in Shared, because it brings the possibility of adding custom data for Console-Apps to. For ASP.NET projects I am playing with the thought if we should add an extension method that brings in easier handling to get the HttpContext back in.

Just to make sure we are on the same track with this: The purpose of the GetCustomData with HttpContext is usability, right?

If so, I am thinking about a more extensible implementation to use it for more types not just HttpContext. For this purpose I´ve thrown together a example:

In Settings we currently have the GetCustomData Property that keeps the Action that sets CustomData in a very general way. What if we introduced another property:

public Dictionary<Type, Action<Exception, object, Dictionary<string, string>>> TypedGetCustomData { get; set; }

We set this property via a Extension method that will be implemented in the specific libs (for example Asp.net or asp.net core). This could be achieved through ah extension method for the Settings class:

public static void SetGetCustomData(this Settings settings, Action<Exception, HttpContext, Dictionary<string, string>> method)
{
         settings.TypedGetCustomData.Add(typeof(HttpContext), (ex, obj, data) => method(ex, (HttpContext) obj, data));
}

To set the method that should be used for customdata this code would be used:

Settings.Current.SetGetCustomDataForHttpContext((ex, context, data) =>
{
        data["SpecificHeader"] = context.Request.Headers["Random"];
});

To use the TypedGetCustomData Dictionary we now would have to add a extension method for error:

private static void AddCustomDataForType(this Error error, Exception exception, HttpContext context)
{
        if (Settings.Current.TypedGetCustomData != null)
        {
                try
                {
                    var method = Settings.Current.TypedGetCustomData[typeof(HttpContext)];
                    method(exception, context, error.CustomData);
                }
                catch (Exception cde)
                {
                    exception.AddLogData(Constants.CustomDataErrorKey, cde.ToString());
                }
        }
}

Which would then be called inside the specific Log Method that sits in the specific lib (again asp.net or asp.net core)

   public static Error Log(
            this Exception ex,
            HttpContext context,
            bool? appendFullStackTrace = null,
            bool rollupPerServer = false,
            Dictionary<string, string> customData = null,
            string applicationName = null)
        {
            if (Settings.IsLoggingEnabled)
            {
                try
                {
                    // Legacy settings load (deserializes Web.config if needed)
                    ConfigSettings.LoadSettings();

                    // If we should be ignoring this exception, skip it entirely.
                    if (!ex.ShouldBeIgnored(Settings.Current))
                    {
                        // Create the error itself, populating CustomData with what was passed-in.
                        var error = new Error(ex, applicationName, appendFullStackTrace, rollupPerServer, customData);
                        // Get everything from the HttpContext
                        error.SetProperties(context);
                        error.AddCustomDataForType(ex, context);
                        if (error.LogToStore(ErrorStore.Default))
                        {
                            return error;
                        }
                    }
                }
                catch (Exception e)
                {
                    Trace.WriteLine(e);
                }
            }
            return null;
        }

So in the end the user would only have to call the SetGetCustomDataForHttpContext method on settings.

In a further step we could also move the AddCustomDataForType to Shared if we use a generic paramter like this:

public static void AddCustomDataForType<T>(this Error error, Exception exception, T context)
{
            if (Settings.Current.TypedGetCustomData != null)
            {
                try
                {
                    if (Settings.Current.TypedGetCustomData.ContainsKey(typeof(T)))
                        throw new Exception("Should we throw here?");
                    var method = Settings.Current.TypedGetCustomData[typeof(T)];
                    method(exception, context, error.CustomData);
                }
                catch (Exception cde)
                {
                    exception.AddLogData(Constants.CustomDataErrorKey, cde.ToString());
                 }
         }
}

I am not sure if we should throw the exception here or just perform a simple check and do no further operations if the type is not included in the Dictionary.

So, in the end, the only code that couldn´t be shared is the Setterextension for the Settings object and the specific call within the Log method.

Another benefit like i already mentioned is the possibility of adding CustomDataGetter for different types. Because of lack of an better example check this one:

public static void SetGetCustomDataForThread(this Settings settings, Action<Exception, Thread, Dictionary<string, string>> method)
{
            settings.TypedGetCustomData.Add(typeof(Thread), (ex, obj, data) => method(ex, (Thread) obj, data));
}

And here is the code for the instantiation of the Settings

Settings.Current.SetGetCustomDataForThread((ex, thread, data) =>
{
     data["ThreadName"] = context.Request.Headers[thread.Name];
 });
NickCraver commented 6 years ago

@StefanKert I thought about that, but the problem is you run into the following:

In short: HttpContext.Current works in old ASP.NET and resolving the dependency works in ASP.NET Core. I just don't think we necessarily need this anymore at all. We only need an example of how to do each in the docs. After Category, I'll spin up the ASP.NET Core lib and sample and see how clean this can look...likely tonight or Wednesday.

This is just one of those cases where complicated code isn't really the right solution to the problem, I think simple examples will suffice. They'll be in (or a description that links from) the upgrade/breaking changes notes as well.

It may be that this gets eliminated altogether, and combined with OnBeforeLog in a different way, given that the HttpContext bits are gone. Still playing, just paused to get Commands in last night :)

StefanKert commented 6 years ago

Agreed. I had a few other solutions in mind, but most of them would be really complicated. This was the one that was the least complicated that came into my mind but your totally right. Like I mentioned before the only benefit of this from my point of view is the small improvement in usability, which goes hand in hand with a more complex and error-prone system.

I am really curious about the plans you have with the Commands and how you want to integrate those things. Really looking forward! Maybe I can add something :), but from the past I know that it is pretty difficult to bring something up that you haven´t already thought of but maybe I can add something in the future!

Anyway, I wanted to thank you that you are taking the time to give me feedback on my suggestions and for being so friendly. This really helps and motivates me! Thanks a lot!

NickCraver commented 6 years ago

@StefanKert If you pull latest a decent start on ASP.NET Core should be in - I want to close this out so that I can remove the v2 branch which 2 people have noted is causing understandable confusion. How about we open an issue for ASP.NET Core functionality and discuss new items/ideas there?

StefanKert commented 6 years ago

@NickCraver Sounds good to me! Thanks for the work. Should we create a separate issue or continue the discussion in #85