Remove extension methods

[nulltoken]

Preliminary step to #1076

[ethomson]

Why Extension methods? As a LibGit2Sharp developer, I find it really hard to find these overloads when they're off hiding if DiffExtensions.cs instead of Diff.cs...

Just my $0.02.

[nulltoken]

Why Extension methods?

Well, most of the overloads already live in xxxExtensions.cs files.
Besides, I find that helps keeping the files holding the concrete logic less lengthy and easier to navigate.
Somehow, I see them as an additional/optional layer on top of the real API.

However, I agree that none of those arguments are pure, undeniable and objective ones. As such, maybe would it be the proper time to make a final decision (BTW, I thank you @ethomson for having raised this before I'm done coding this PR 😉)

Who's in favor of xxxExtensions? Who think they bring more pain than value?
Should we keep them or 🔫 🔥 💀 them?

/cc @bording @carlosmn @dahlbyk @jamill @jeffhostetler @shiftkey @whoisj

[Therzok]

feels ignored

I'm in favor of Extension methods. I've never been a fan of all overloads functions in one place, especially when they're convenience functions provided to transform some parameters into others, or use default values in place of parameters aka just wrappers.

[whoisj]

Generally I'm either for or agnostic of extension methods in C# due the way the compiler and runtime work to make it "just work". However, I do not believe we should be moving virtual methods from an inheritable class into extension methods. Doing so effectively takes control away from the inheritor.

While the inheritor could have public override Branch Add(string name, string committish) now they cannot because the method is defined elsewhere an in a non-override-able way (unless the language has provided functionality that I'm unaware of). This reduces the utility of the library and makes me sad.

I am a little concerned about calling partial class implementations xxxExtensions when they are not actually extensions, but partial implementations. I think the CLR guys do this best when they use the ClassName.FunctionalityGroup.cs naming convention. They can then group all of sub-files under a single entry in the Solution Explorer though XML hackery - which makes finding stuff actually easier.

So while this is cleaner, I sadly feel I need to 👎 the change as is. 😞

[ethomson]

I've never been a fan of all overloads functions in one place...

Why extensions, then, and not partial classes?

[carlosmn]

If it is about separating the implementation into multiple files so we have the core plus convenience, functions, I agree that partial classes sound like a much better match. Extension methods are made so you can inject methods into external code, but we own all of libgit2sharp; partial classes would allow for the code to be split across files but still clearly signal that it's our code.

[Therzok]

While the inheritor could have public override Branch Add(string name, string committish) now they cannot

Thing is, why would you have different behaviours between string commitish and Branch b, since both resolve to the same thing? Only one method should be virtual, possibly the one which is called into from all extensions and don't allow overriding the public function, but add a virtual protected OnAdd(param1 param2).

Why extensions, then, and not partial classes?

That's even better, imo, I'm up for this.

[whoisj]

@carlosmn in many C# projects, extension methods are also used to implement methods which cannot be overridden as well. Not that I agree with that, but it does enable the creation of partially sealed classes.

That said, I don't think that this is the point of this PR and I full agree that partial classes are likely a better solution.

[whoisj]

Thing is, why would you have different behaviours between string commitish and Branch b, since both resolve to the same thing? Only one method should be virtual, possibly the one which is called into from all extensions and don't allow overriding the public function, but add a virtual protected OnAdd(param1 param2).

Why not? What if a service does OI logging and they wanted to hook one of the methods and add a logging event in it? What if an implementation wanted to prevent the use of commitish and wanted to replaced one of the methods with a throw new NotImplementedException to make sure none of their devs used it?

We cannot pretend to know how the library will be used. We can only provide the most useful library possible.

[nulltoken]

So partial classes would be a 👍?

[whoisj]

So partial classes would be a 👍?

I do believe that is the consensus. 😄

[dahlbyk]

I don't see much point in breaking out partial classes, TBH.

[ethomson]

@dahlbyk Meaning everything in one file, or you prefer the status quo using extensions?

[dahlbyk]

I haven't struggled with the extensions, personally, so I lean toward:

1. Members to implement interfaces (duh).
2. Members for unique "full" method signatures (typically the ones with options).
3. Extensions for convenience overloads with reasonable defaults.

I'm not sure how much I buy the "what if we need to inherit?" argument because we return concrete implementations throughout the code. Outside of testing I'm not sure how it would ever be useful to override our virtual members.

[shiftkey]

Why extensions, then, and not partial classes?

I always come back to the canonical use case for partial classes - codegen. Beyond that I'm generally skeptical about it's value - perhaps there's some underlying issue which is being masked by extracting partial classes? Maintainability? Complexity?

Extension methods are okay, but again, my favourite use case for them is "for types you don't own". Beyond that, I'd push back and question what problem we're trying to solve here.

[nulltoken]

So partial classes would be a 👍?

I do believe that is the consensus. 😄

Indeed, fastest consensus reach ever. 😜

[nulltoken]

As it looks like I'm the only one who dislikes overloads living next to the concrete implementation, how about just getting rid of the xxxExtensions.cs files and moving everything from them back in xxx.cs files?

[dahlbyk]

I would be fine with folding xxxExtensions into the "parent" class with the possible exception of classes that implement interfaces. Then again, our interfaces really aren't meant to be implemented externally, so it may not matter if it would be tedious to reimplement them.

[nulltoken]

@dahlbyk Good point regarding the interfaces.

I would be fine with folding xxxExtensions into the "parent" class

Do anyone oppose to this?

[nulltoken]

I've started to work along @dahlbyk's proposal. How does it look?

I've also taken the opportunity to surreptitiously introduce some breaking changes.

[whoisj]

Seems reasonable to me. I do prefer concrete implementations to extensions. 👍

[dahlbyk]

Convert * into a property

Are these adjustments worth a breaking change? Or do we not really care until stabilization is complete (I'm out of the loop on those discussions)?

[nulltoken]

I think this is now done. Only remaining points are some submodule status related extensions (hence the failing build). They actually look like unused, which is weird. What should we do with them?

/cc @dahlbyk @jamill

[whoisj]

They actually look like unused, which is weird. What should we do with them?

I'd like to keep them as I have plans to use, at least some of, them.

[dahlbyk]

What should we do with them?

Add tests for them, exclude enum extensions in meta test?

[nulltoken]

Add tests for them, exclude enum extensions in meta test?

I'm not sure I'd be able to add interesting tests (other than copying the implementation).

How about I mark, for now, the enum as internal and let @whoisj add the tests when he actually needs the enum? 😈

[whoisj]

How about I mark, for now, the enum as internal and let @whoisj add the tests when he actually needs the enum? 😈

That should be fine. It'll be a few weeks before I start on my submodule stuff - no point in using your time now for something I'll need only then. 😊

[nulltoken]

How about I mark, for now, the enum as internal

That should be fine.

Done. Ready for final review.

no point in using your time now for something I'll need only then

@whoisj 😍

[jamill]

If someone actually uses this API they will be broken when we remove it. While we are pretty good about testing APIs, surely this isn't the only one missing test coverage, so I don't know if missing test coverage is reason enough to remove it.

[nulltoken]

@jamill It's not a definitive removal. It'll be reintroduced as soon-ish by @whoisj.

[nulltoken]

Beside we can't publish anything until #1091 is fixed.

[nulltoken]

@jamill Regarding code coverage, you're right. I've logged #1106 to not forget about it

[nulltoken]

@dahlbyk 💎 ‼️

[dahlbyk]

Pushed a rebound, feel free to discard. I don't see a reason to drop these methods for lack of test coverage in a PR about removing invalid extension methods. If extensions on an enum are considered valid (and they should be, IMO), then we might as well test accordingly.

[nulltoken]

Pushed a rebound, feel free to discard.

I won't. Your approach is far better. Thanks for this!