Skip to content
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

Throw when xxxOptions are null #1076

Open
nulltoken opened this issue Jun 6, 2015 · 6 comments

Comments

Projects
None yet
3 participants
@nulltoken
Copy link
Member

commented Jun 6, 2015

From https://github.com/libgit2/libgit2sharp/pull/1068/files#r31650592

We rely on the following pattern throughout the codebase.

public Things DoThings()
{
     return DoThings(null);
}

public Things DoThings(ThingsOptions options)
{
     options = options ?? new ThingsOptions();
     [...]
}

Let's change this to the following one

public Things DoThings()
{
     return DoThings(new ThingsOptions());
}

public Things DoThings(ThingsOptions options)
{
     Ensure.ArgumentNotNull(options);
     [...]
}
@Therzok

This comment has been minimized.

Copy link
Member

commented Jun 6, 2015

This would introduce API contract breakage (huge one) and code verbosity on user side. I for one am against this change.

@nulltoken

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2015

The table below represents the impact of the proposed change.

Code Current Impl. Proposed Impl.
DoThings();
DoThings(new Options());
DoThings(null); 💥

I agree about the API code contract breakage. However, now is the time for this (hence the stabilization label). v0.22 is taking quite a long time to get out of the woods as we're trying to obsolete as many things as possible in order to be able to release v1.0 as the next version after it.

Regarding the additional code verbosity, I'm not sure to see what you refers to. Could you please elaborate further on this topic? Maybe with an example?

From another standpoint, I'm not sure I see the added value of allowing DoThings(null) code construct.

@Therzok

This comment has been minimized.

Copy link
Member

commented Jun 6, 2015

Ah, so, basically, you wouldn't be able to pass null, but the overload will construct new default options. I get it, and it works.

Probably all the overloads should go into extensions anyway.

@Therzok

This comment has been minimized.

Copy link
Member

commented Jun 6, 2015

Aka:

public Things DoThings()
{
     return DoThings(new ThingsOptions());
}

Should be in a:

public static class ThingsExtensions
{
    public Things DoThings(this ThingClass things)
    {
        return new things.DoThings(new ThingsOptions());
    }
}
@nulltoken

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2015

Yes. That was the idea 😉

@gistofj

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2015

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.