Prefer interfaces instead of static factories

[HarelM]

The following is impossible to fake in tests: Site.CreateAsync(...) which means that if I want to using this client in my code and be able to test my code without doing calls to wiki I'll need to wrap it with my code, which, IMHO, kinds of misses the point a bit. A better API would provide factories for all the object so that I'll be able to mock the factory in my tests. For the example above the following would do wonders when trying to test code that uses this library:

public class SiteFactory : ISiteFactory {
    public Task<Site> CreateAsync(...) { ... }
}

I don't mind helping out with the relevant code changes using pull requests, but I need an initial approval that these changes are acceptable.

[CXuesong]

This is a feasible approach. However, I'm afraid it might introduce more code both on the library and on client side. Just think of Task.Run. There is no need to provide an abstract factory as you've illustrated, because there is only one WikiSite class in the whole library, providing the basic funtionality: construct a WikiSite instancce.

If you have your own WikiSite-derived class (which is not so often, I suppose), you may write your own static factory method in another class.

If you want some abstract-factory-like behavior, you may write a interface in your code, and call WikiSite.CreateAsync in your default implementation. Actually, that's a little bit similar to what I did in my test cases.

https://github.com/CXuesong/WikiClientLibrary/blob/0ef6c62a3feedc4bb7cbc2708ca92238fb467d7e/UnitTestProject1/WikiSiteTestsBase.cs#L82-L95

[HarelM]

Task.Run is a very basic framework method I don't think you should compare to it. Also you can provide both the static code for fast use and the factory for better testability, doesn't have to be mutually exclusive. The fact is that WikiClient and Site are what I define as "DataAccessLayer" code since it does http requests and all DataAccessLayer classes in my code have interfaces so that I will be able to run my unit tests without sending any http requests - this is extremely important in order for the test not to fail and run fast. Also Task has TaskFactory...

[CXuesong]

Okay this is really a hard decision to make… I'm still reluctant on it :pensive: Because from my library's perspective, I don't want to get involved in the factory-things prematurely. And though it might be not so correct but this is intrinsically a JSON client. To test if it works, it's better to test it with some real API endpoints, testing whether the logic is corrrect. Thus, I'm still inclined to keep the current code as-is.

There is a TaskFactory because (I guess) users might want to use different default TaskScheduler, TaskCreationOptions, etc. And Tasks are created very often, so creating a factory for each set of settings offers benefits. On the other hand, we often work on only 1 or 2 wikis, so using just a simple static factory method is enough. Even if you are working with interwikis, you may use WikiFamily to meet your needs.

[HarelM]

Your call, I ended up wrapping you code with code of my own in order to be able to do things that are specific (?) to my business domain. Relevant code can be found here for reference: https://github.com/IsraelHikingMap/Site/blob/master/IsraelHiking.DataAccess/WikipediaGateway.cs

[CXuesong]

Okay, I see that. So you abstracted your Wiki-operations into interface, so later you can mock its implementation, I guess? Thank you for your understanding 🌚

This issue may be reopened later if the situation changes.