-
Notifications
You must be signed in to change notification settings - Fork 77
Conversation
2caffa2
to
c0cfe23
Compare
Moved some files around to the right namespaces and locations
c0cfe23
to
00de3e5
Compare
} | ||
} | ||
|
||
public readonly struct InternalCommandRequestId : IEquatable<InternalCommandRequestId>, IComparable<InternalCommandRequestId> |
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.
What is the difference between InternalCommandRequestId
and CommandRequestId
? Is it that they're two different wrappers for two different types of commands and could theoretically deviate from each other? Or are you trying to create type safety?
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.
They are the same data wise, but 1 is an id the GDK generates when you schedule a command to be send. And the internal ID is the ID the worker SDK returns when the command is actually send over the network.
Using 2 different types here ensures they aren't accidentally mixed up,
@@ -65,11 +65,16 @@ private void SendPlayerCreatorEntityQuery() | |||
|
|||
private void HandleEntityQueryResponses() | |||
{ | |||
if (!playerCreatorEntityQueryId.HasValue) |
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.
What does it not having a value mean has happened? How has that changed with the rest of the code, or is this something you found because of the change?
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.
Previously a check was done if(long? == long)
which worked because there is an implicit conversion there. This implicit conversion is gone.
If there is no Value in that nullable nothing is supposed to happen, so I changed it to an early out.
{ | ||
public readonly struct CommandRequestId : IEquatable<CommandRequestId>, IComparable<CommandRequestId> | ||
{ | ||
public readonly long Raw; |
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.
So... does this need to be public?
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.
For us no, but I wanted to keep this public in case someone used the command ID's for something.
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.
But then they can use this right? It has equality and comparison operators. Like why would you want the raw number over the wrapper struct?
Co-authored-by: Jamie Brynes <jamiebrynes@improbable.io>
Split file with multiple interfaces into multiple files Fix sonar code smells
Kudos, SonarCloud Quality Gate passed! 0 Bugs |
Description
Make command request ID's typed.
Split interfaces for commands into their own files.
Small drive-by on making comparer classes sealed.
Documentation