tparviainen / clashofclans Goto Github PK

Start to use .NET dependency injection.

When using DI it allows to refactor the existing implementation in several ways such us:

• Allows to use ILogger<T>
• Allows to use options pattern for configuration

The implementation should provide extension method (AddClashOfClans) for clients to call, which configures everything automatically. After adding Clash services to IoC the clients can inject IClashOfClans (or fine grained interfaces (IClans, ILocations, ILeagues, IPlayers, ILabels) to access specific data) to classes where the functionality is needed.

With DI the way to add Clash of Clans to any project would be:

services.AddClashOfClans(config =>
{
    config.Tokens = [tokens];
    config.MaxRequestsPerSecond = ...;
});

Additional links:

• https://xfischer.github.io/logging-dotnet-core/

Open items:

• Support other than .NET dependency injection, for example Ninject?
• Should there be a possibility to use the library in a traditional way as well i.e. without DI?
• How to change token on the fly?

The link is for https://docs.microsoft.com/fi-fi/nuget/nuget-org/package-readme-on-nuget-org

If the plan is to use the same README.md as in the repository then the content needs to be updated, for example:

• This repository contains --> README.md no longer exists only in repository context!
• Check what image links are supported in NuGet.org context

Supercell fixed the bug regarding LabelIds and now it is possible to implement validator for LabelIds because the correct format of the query parameter is known.

Currently DocFX files are in the root of the project and there are more and more files in the root and it does not look good. Instead of keeping DocFX files scattered in the root they should be moved to own folder (for example docfx_project) and only the mandatory files (.nojekyll) and folders (docs) should be kept in the root.

When doing this change also the DocFX workflow must be updated to reflect refactored project structure.

Study also the gh-pages way of hosting Github Pages.

COC API documentation states ClanMember type has TownHallLevel: integer, could we have it here as well? Thank you!

SC has recently changed the names of the API models. Because of the change the library model names are no longer in sync with SC API model names.

Root element names have been changed as well as support for localized names for leagues, error response, etc.

This change, once implement will be a breaking change for existing library users!

• /capitalleagues = List capital leagues
• /capitalleagues/{leagueId} = Get capital league information
• /locations/{locationId}/rankings/capitals = Get capital rankings for a specific location

Describe the bug
When API method is called and the response is awaited immediately then the implementation of throttling limit works properly. Next is an example when the current implementation works properly:

var clan = await coc.Clans.GetAsync(clanTag);
var warLog = await coc.Clans.GetWarLogAsync(clanTag);
var player = await coc.Players.GetAsync(playerTag);

However when async methods are not awaited during the call, instead they all are awaited later on the current implementation does not work. Next is an example where the current implementation fails:

var players = new List<Task<PlayerDetail>>();
for (int i = 0; i < clan.Members; i++)
{
    players.Add(coc.Players.GetAsync(clan.MemberList[i].Tag));
}
await Task.WhenAll(players);

This leads to exception "Request throttling limits exceeded" even though the throttling limit (default 10 requests per second) is way below the allowed limit.

Currently the paging works in a way that the client is responsible for setting the Query.After property based on the value from Paging.Cursors.After property. This implementation should be changed so that from API client point of view things should happen "automatically" without the user needing to set those properties manually.

There are few ways that the client can use the API and the paging should work whether the API is used in either way. Currently the ways to use the API are:

var clans = (ClanList)await _coc.Clans.SearchClansAsync(query);
clans // ClanList

var searchResult = await _coc.Clans.SearchClansAsync(query);
searchResult.Items // ClanList

Both of those usage examples should be covered in the improved paging implementation and to provide nice and clean way to use the paging feature.

Remove obsolete APIs and related entities because the API no longer exists

• GetClanVersusRankingAsync, use GetClanBuilderBaseRankingAsync instead
• GetPlayerVersusRankingAsync, use GetPlayerBuilderBaseRankingAsync instead

Clash has a feature called clan capital, add a support for clan capital related data in API.

https://docs.microsoft.com/en-us/dotnet/core/extensions/logging?tabs=command-line

Implement a basic support for Discord, where the Clash of Clans NuGet library provides some services for Discord (for example via ASP.NET Core backend). The implementation should be easy to duplicate so that it is usable/extendable for others as well. The implementation should work in a Docker container, which makes it more portable for different platforms.

Describe the bug
GitHub bug ...

To Reproduce
Steps to reproduce the behavior:

1. git checkout main
2. git push

$ git push
Total 0 (delta 0), reused 0 (delta 0), pack-reused 0
remote:
remote: Heads up! The branch 'main' that you pushed to was renamed to 'main1'.
remote:
To https://github.com/tparviainen/clashofclans.git
   2a0cff0..a1c272d  main -> main

According to ConfigureAwait FAQ article the recommendation for 'general-purpose library' is to use ConfigureAwait(false).

This change will bring a slight performance benefit and should not have any disadvantages compared to existing implementation (that implicitly uses ConfigureAwait(true)).

Even though there is no SynchronizationContext in ASP.NET Core the library might be used from another app that has a context and thus ConfigureAwait(false) should be used in case the asynchronous API method is executed synchronously.

Currently unit/integration test method names do not follow specific naming convention. When a test fails, without reading a test code, one should understand what he/she broke.

https://github.com/dotnet/aspnetcore/wiki/Engineering-guidelines#unit-test-method-naming
Unit test method names must be descriptive about

1. What is being tested
2. Under what conditions
3. What is the expected result

https://enterprisecraftsmanship.com/posts/you-naming-tests-wrong/
https://github.com/dotnet/interactive
https://docs.microsoft.com/en-us/dotnet/core/testing/unit-testing-best-practices#naming-your-tests

Good test names:

• Aim for test names that describe behavior not implementation, name should answer to question "what the test tests".
• Test name should describe the behavior of the AAA triplet (Arrange/Act/Assert)
• Test method name should give enough information for a reader to understand what is broken without reading the test code.
• Some people use tests for learning purpose of how the code works
• Assert message provides the details (expected vs actual result)

It seems that the naming conventions for tests is wide, for Clash of Clans library there are only two things to consider when naming a test:

• Aim for test names that describe behavior not implementation.
• When a test fails, without reading a test code, one should understand what he/she broke.

As suggested in this video https://youtu.be/TJiLhRPgyq4 it is a good idea to multi-target the library for .NET Standard 2.0 and .NET Core 3.0 and implement a support for "nullable reference types".

https://devblogs.microsoft.com/dotnet/embracing-nullable-reference-types/
https://devblogs.microsoft.com/dotnet/try-out-nullable-reference-types/
https://www.meziantou.net/csharp-8-nullable-reference-types.htm
http://www.roji.org/nullable-reference-types-with-old-tfms

'At least one filtering parameter must exist: badRequest'

ClashOfClansException message when searching a clan by tag using .GetAsync

How to reproduce

try-catch the .GetAsync function inside a async Task and print to console the clash of clans exception

Versions

• ClashOfClans 3.0.4

https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclient?view=netstandard-2.0

HttpClient is intended to be instantiated once and re-used throughout the life of an application. Instantiating an HttpClient class for every request will exhaust the number of sockets available under heavy loads.

The issue is pretty well visible when executing next command when there are API operations ongoing:

netstat -ano -p TCP

Currently there are several appsettings.json files for example one under console app, blazor app and integration tests which makes it really time consuming to update those files once for example token changes. Improve the settings handling in a way that there is only one location that needs to be updated once token changes.

Currently the ClashOfClansClient API supports only one token per API instance. It would be good if the API would support multiple tokens and automatically select different token for different requests (for example round robin fashion).

Supporting multiple tokens allows clients to bypass throttling limits of the SC API in the client services where throttling limits with one token are causing issues.

• See dotnet/docs#34893

Describe the bug
It throws an error when trying to get informations about a player.

To Reproduce
Call any method to get player information, for example
await client.Players.GetPlayerAsync(OWNER_TAG);

Expected behavior
To parse correctly the village

Execution Environment (please complete the following information):

• .NET version: .NET 6
• ClashOfClans NuGet version: 8.8.0

Proposal correction
As far as I've seen, simply add ClanCapital into Village enum

Additional context

Describe the bug
Models (Clan, Player, ...) contains many properties that have been declared as nullable value types, however many of those are properties that in practice cannot contain null values and are always initialized by the Supercell API.

One way to fix this is to change the existing nullable value type to one with nullable backing field but with non nullable property, this allows to track when the content is received as 'null', which allows to adjust the implementation accordingly in the future. This needs a slight change to GetNullProperties to loop through fields as well.

This will be a breaking change!

Describe the bug
It seems ClashOfClans -Version 8.8.3 does not have latest change included yet. Could we have that updated. Thank you!

Currently all tests are in one test project. Some of the tests are unit tests (query, validator) but most of them are integration tests.

What would be the best approach for these tests?

• Own project for integration tests and own project for unit tests?
• Separate different tests using namespaces but keep them in one project?
• Something else?

What are the benefits keeping tests in one project vs. separating them?

• Unit tests can be executed without valid SC token.