-
Notifications
You must be signed in to change notification settings - Fork 2
🌸 2.0.0 #43
base: master
Are you sure you want to change the base?
🌸 2.0.0 #43
Conversation
Co-authored-by: AoshiW <48525283+AoshiW@users.noreply.github.com>
public ILogger Logger { get; set; } = LoggerFactory | ||
.Create(c => | ||
{ | ||
c.AddConsole(); | ||
c.SetMinimumLevel(LogLevel.Debug); | ||
}) | ||
.CreateLogger("RiotBlossom"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should set it to NullLogger
, it is better if the user sets it up himself
Currently it has several problems, for example:
- inability to change
LogLevel
- inability to remove the current Console Logger
- inability to add another Logger Provider
protected async override Task<string?> ReadAsync(string key) | ||
{ | ||
var json = _cache[key]; | ||
|
||
return await Task.FromResult(json) | ||
.ConfigureAwait(false); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method does not have to be async
protected async override Task<string?> ReadAsync(string key) | ||
{ | ||
return await Task.FromResult<string?>(null) | ||
.ConfigureAwait(false); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method does not have to be async
} | ||
} | ||
|
||
protected abstract Task<string?> ReadAsync(string key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the return type could be ValueTask<string?>
public async override Task ProcessRequestAsync(DataCall call, HttpRequestMessage req) | ||
{ | ||
// NO-OP | ||
await Task.CompletedTask | ||
.ConfigureAwait(false); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method does not have to be async
var delay = delays | ||
.OrderByDescending(x => x.Ticks) | ||
.First(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps MaxBy
could be used.
I'll be keeping this PR here until I finish it. Should solve issues with rate limiter and library complexity. Will be bumping framework requirement to the upcoming .NET 8 LTS release. 💚