The Wayback Machine - https://web.archive.org/web/20190721023526/https://github.com/libgit2/libgit2sharp/issues/1076
Skip to content

/libgit2sharp

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

Labels
Easy fix Stabilization Up for grabs
3 participants
@nulltoken @Therzok @gistofj
@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);
     [...]
}

@nulltoken nulltoken added Up for grabs Easy fix Stabilization labels Jun 6, 2015

@nulltoken nulltoken referenced this issue Jun 6, 2015

Merged

Introduce StashCollection.Apply and Pop #1068

@Therzok

This comment has been minimized.

Sign in to view
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.

Sign in to view
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.

Sign in to view
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.

Sign in to view
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.

Sign in to view
Copy link
Member Author

commented Jun 6, 2015

Yes. That was the idea 😉
@gistofj

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Jun 6, 2015

👍

@nulltoken nulltoken referenced this issue Jun 15, 2015

Merged

Remove extension methods #1093

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.