rewrite #17
rewrite #17
Conversation
| /// </item> | ||
| /// </list> | ||
| /// </typeparam> | ||
| public abstract class AClientBaseTyped<T> : AClientBase where T : IDisposable |
There was a problem hiding this comment.
This class seems like unnecessary overhead. I believe you could move everything to AClientBase, and make it generic with IDisposable restriction.
There was a problem hiding this comment.
I think the code will be more simple like that. You will get rid of that abstract SetSpecificClient method.
| @@ -1,15 +1,22 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
|
|
|||
| <PropertyGroup> | |||
| <TargetFramework>net6.0</TargetFramework> | |||
| <TargetFramework>net7.0</TargetFramework> | |||
There was a problem hiding this comment.
I would stay on .net 6, who knows what will happen to other libraries if this suddenly changes to .NET 7. Also it breaks automatic checks here.
There was a problem hiding this comment.
Yeah. I saw that within the checks that failed and I'm going to revert it
There was a problem hiding this comment.
In theory it wouldnt be an issue bcs thats just the Test project nobody should really upon but I guess I would keep it at .NET 6 as that is the LTS right now
| { | ||
| Limit_20_in_30_Seconds = 20, | ||
| Limit_100_in_30_Seconds = 100, | ||
| Limit_7500_in_30_Seconds = 7_500, |
There was a problem hiding this comment.
It's a notation, that makes large numbers more readable.
The underscore can be read like a thousand separator.
There was a problem hiding this comment.
It's "new" thing from C# 7.0, it's great, especially when dealing with large numbers. The compiler basically ignores those underscores, but for us humans it's much more readable.
| // please take a look at this enums summary | ||
| Limit_100_in_60_Seconds = 50 | ||
| } | ||
| } |
There was a problem hiding this comment.
Whispers are no longer in IRC and this is not needed.
There was a problem hiding this comment.
May/Should i remove all stuff related to whispers?
There was a problem hiding this comment.
If you're going to remove it, I would remove it only from this PR. If there is more whispers code in the library, it can get messy, and it would be better to have it in a separate PR.
There was a problem hiding this comment.
Yeah. I got it.
I'm going to remove the internal stuff and to mark the public ones as obsolete.
| bool Send(string message); | ||
|
|
||
| /// <summary> | ||
| /// Queue a Whisper to Send to the server as a String. | ||
| /// Queue a Whisper to Send to the server as a String. |
There was a problem hiding this comment.
Again whispers no in IRC anymore
the values wont change at runtime
and update version to 2.2.0 PubSub has its own PING/PONG-Timers PubSub only subscribes to Events PubSub only receives push-messages from twitch
changed visibility from internal to protected comments updated
so send has to be synchronized/locked
| <Company>swiftyspiffy, Prom3theu5, Syzuna, LuckyNoS7evin</Company> | ||
| <PropertyGroup> | ||
| <TargetFramework>netstandard2.0</TargetFramework> | ||
| <VersionPrefix>1.1.0</VersionPrefix> |
There was a problem hiding this comment.
This is going to to be a major release and probably should be V2.0.0
There was a problem hiding this comment.
That is just an oversight by Christian.
Everywhere else it already is called version 2.0.0. but still a good catch
There was a problem hiding this comment.
No need to be sorry.
Thinking about how often we had to issue patches bcs of a missnamed property or something like that we can hardly judge :D
ghost
marked this pull request as ready for review
| @@ -31,7 +31,7 @@ public abstract class AClientBase<T> : IClient where T : IDisposable | |||
| private static readonly object LOCK = new object(); | |||
|
|
|||
| #region properties protected | |||
| protected ILogger LOGGER { get; } | |||
| protected ILogger? LOGGER { get; } | |||
There was a problem hiding this comment.
why are those completely uppercase?
| @@ -31,7 +31,7 @@ public abstract class AClientBase<T> : IClient where T : IDisposable | |||
| private static readonly object LOCK = new object(); | |||
|
|
|||
| #region properties protected | |||
| protected ILogger LOGGER { get; } | |||
| protected ILogger? LOGGER { get; } | |||
| protected abstract string URL { get; } | |||
There was a problem hiding this comment.
why are those completely uppercase?
| @@ -31,7 +31,7 @@ public abstract class AClientBase<T> : IClient where T : IDisposable | |||
| private static readonly object LOCK = new object(); | |||
There was a problem hiding this comment.
why are those completely uppercase?
| @@ -198,8 +200,11 @@ public bool Send(string message) | |||
| LOGGER?.TraceMethodCall(GetType()); | |||
| try | |||
| { | |||
| SendIRC(message); | |||
| return true; | |||
| lock (LOCK) | |||
There was a problem hiding this comment.
is it wise to do a "normal" object lock vs e.g. using a SpinLock or a SemaphoreSlim?
| @@ -27,10 +27,11 @@ namespace TwitchLib.Communication.Clients | |||
| /// </item> | |||
| /// </list> | |||
| /// </summary> | |||
| public abstract class AClientBase : IClient | |||
| public abstract class AClientBase<T> : IClient where T : IDisposable | |||
There was a problem hiding this comment.
Not sure if I missed a recent convention but can we rename that to just ClientBase or sth and drop the A?
| @@ -27,10 +27,11 @@ namespace TwitchLib.Communication.Clients | |||
| /// </item> | |||
| /// </list> | |||
| /// </summary> | |||
| public abstract class AClientBase : IClient | |||
| public abstract class AClientBase<T> : IClient where T : IDisposable | |||
| { | |||
| #region properties protected | |||
| protected ILogger LOGGER { get; } | |||
There was a problem hiding this comment.
why are those completely uppercase?
|
|
||
| /// <summary> | ||
| /// this <see langword="class"/> bundles almost everything that <see cref="TcpClient"/> and <see cref="WebSocketClient"/> have in common | ||
| /// its not generic/it has no <see cref="Type"/>-Parameter, |
There was a problem hiding this comment.
This description seems no longer true. This class has generic T type.
|
|
||
|
|
||
| #region properties public | ||
| /// <summary> |
There was a problem hiding this comment.
I would change this sumary as well. T can be any client implementing IClient.
| ILogger? logger = null) | ||
| { | ||
| LOGGER = logger; | ||
| // INFO: Feedback by Bukk94: not to restrict the Client to those two known types |
There was a problem hiding this comment.
I guess you can remove those comments as they are no longer needed.
| LOGGER?.TraceAction(GetType(), "try to connect"); | ||
| if (!first) | ||
| { | ||
| Task.Delay(Options.ReconnectionPolicy.GetReconnectInterval()).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
You put a lot of effort into reworking whole project (kudos to you!). Have you considered changing these .GetAwaiter().GetResult() into proper async/await? Or you don't want breaking changes? Or do you plan to do it in separate PR?
What is your opinion in this @Syzuna?
There was a problem hiding this comment.
I was thinking about that as well.
I mean the whole thing is basically a breaking change already it has some API changes that propagate up to public user API space in TwitchLib.Client and TwitchLib.Pubsub anyway so we can go the full distance
There was a problem hiding this comment.
That's true. But I would go for second PR on this change as well. You don't have to merge into master and release immediately right? This can go in several PRs. This one, then async/await, then nullability change. It's always better to have several smaller PRs than one giant one.
There was a problem hiding this comment.
A commit/merge to master doesnt kick of a release.
It only targets master bcs I was an idiot myself and commited sth directly to master instead of dev and I didnt sync back up dev yet.
wanted to do that after this PR is merged.
| <TargetFramework>netstandard2.0</TargetFramework> | ||
| <VersionPrefix>2.0.0</VersionPrefix> | ||
| <LangVersion>8.0</LangVersion> | ||
| <Nullable>enable</Nullable> |
There was a problem hiding this comment.
Are you sure you want to enable nullable reference types? I think you just added whole lot of extra complexity to your PR. You must append ? to all reference types that are nullable. As result of this small change, there are a lot of warning across your code (for example assigning null to CancellationTokenSource)
There was a problem hiding this comment.
I actually think it is a good idea to enable them and update the code accordingly but maybe this should be a seperate PR
| Task.Delay(MonitorTaskDelayInMilliseconds * 2).GetAwaiter().GetResult(); | ||
| CancellationTokenSource?.Dispose(); | ||
| // set it to null for the check within this.StartMonitorTask() | ||
| CancellationTokenSource = null; |
There was a problem hiding this comment.
Warning: Cannot convert null literal to non-nullable reference type. See my previous comment.
| { | ||
| _reconnectStepInterval = minReconnectInterval; | ||
| _minReconnectInterval = minReconnectInterval > maxReconnectInterval | ||
| reconnectStepInterval = minReconnectInterval; |
There was a problem hiding this comment.
There was no reason to remove underscores (_) from those private fields. Having underscore in the private field is common practice in C#. This change forces you to using this keyword that wouldn't be otherwise necessary.
There was a problem hiding this comment.
yeah I am not that happy with some of the naming as well but that is sth I can fix as well if Christian wants me to do that after the PR is merged
There was a problem hiding this comment.
Agree, it can always be changed later. It just creates extra changes :) Nothing code-breaking though.
And I hope you don't mind I joined the review.
There was a problem hiding this comment.
always happy for more pairs of eyes. especially on big PRs :)
ghost
closed this by deleting the head repository
|
What happened @Christian-Riedel? |
the code is crap. |
Not really. It just needs some adjustments and improvements. Are you planning to improve it or you threw away everything? |
I threw away everything |
No description provided.