removed mutable values and loops. Added railway oriented error handling #8
Conversation
src/s2client-fsharp/Instance.fs
Outdated
| @@ -93,7 +100,8 @@ module Instance = | |||
|
|
|||
| let createGame (instance:Sc2Instance) mapName (participants:Participant list) realTime = async { | |||
| let req = new RequestCreateGame() | |||
| for player in participants do | |||
| participants | |||
| |> List.iter (fun player -> | |||
There was a problem hiding this comment.
I'm not sure if List.iter is that much better than just using a for loop...
There was a problem hiding this comment.
I don't mind either way, happy to revert.
src/s2client-fsharp/ErrorDefs.fs
Outdated
| namespace Starcraft2 | ||
|
|
||
| type ApplicationError = | ||
| |FailedToEstablishConnection of string |
There was a problem hiding this comment.
I'm okish with railway style as long as we properly forward error information (or logging it for that matter) instead of throwing everything away and only keeping the "Message" here.
"Standard" Exception handling has lots of features you now basically need to replicate manually.
| let stepResponse = response.Step | ||
| checkNullAndWarnings response stepResponse | ||
| return response.Status } | ||
| clientSock.ConnectAsync(fullAddress, tok) |> Async.AwaitTask |> Async.RunSynchronously |
There was a problem hiding this comment.
Now the Sc2Connection constructor blocks and is no longer async. Another design might be
- change the constructor itself to
private - add an async static
Create/Connectmethod - add the socket to the constructor parameters.
There was a problem hiding this comment.
There is a connect function later on in the file, that is used to create a Sc2Connection
let connect address port timeout tok = async{
return Sc2Connection(address, port, timeout, tok)
}
Is this in line with your comment?
There was a problem hiding this comment.
No, RunSynchronously blocks the current thread which is not really required here. The problem why you are using it is because it is not possible to use async-code in the constructor. But you can workaround by just adding the connected socket to the constructor arguments and using an async static Connect/Create member and marking the constructor internal (as suggested by the initial comment)
There was a problem hiding this comment.
IE move the getConnectedSocket() function call into connect and await it.
| let sendRequest req = async{ | ||
| return! | ||
| connectedSocket | ||
| |> Result.map getAgent |
There was a problem hiding this comment.
So you create a new agent every time? Are you sure that is what we want?
There was a problem hiding this comment.
Good catch! this is not what we want. Will fix it.
src/s2client-fsharp/Railway.fs
Outdated
| @@ -0,0 +1,101 @@ | |||
| namespace Rail | |||
|
|
|||
| module Result = | |||
There was a problem hiding this comment.
Maybe we should change the namespace and mark that module internal?
src/s2client-fsharp/Sc2Game.fs
Outdated
| |> Result.map Some | ||
| |> Result.map attachPart | ||
| |_ -> None |> attachPart |> Ok | ||
| ) |
There was a problem hiding this comment.
Note: Previous code was parallel while this is sequentially.
There was a problem hiding this comment.
On the one hand I see improvements and elegance in the handling of the protobuf types and the connection handling.
On the other side I feel like the game loop is harder to understand now. Maybe we can get the best of both worlds?
What do you think? (Sorry for the very late review :( )
| x.Connection.Disconnect(exitInstance) | ||
| if (not(x.Process.WaitForExit(1000))) then | ||
| x.Process.Kill() | ||
| {Connection:ProtobufConnection.Sc2Connection; Process:System.Diagnostics.Process} |
| for clientPorts in ports.ClientPorts do | ||
| ports.ClientPorts | ||
| |> | ||
| List.iter (fun clientPorts -> |
There was a problem hiding this comment.
I guess I'd prefer the for loop here as well :)
| let stepResponse = response.Step | ||
| checkNullAndWarnings response stepResponse | ||
| return response.Status } | ||
| clientSock.ConnectAsync(fullAddress, tok) |> Async.AwaitTask |> Async.RunSynchronously |
There was a problem hiding this comment.
No, RunSynchronously blocks the current thread which is not really required here. The problem why you are using it is because it is not possible to use async-code in the constructor. But you can workaround by just adding the connected socket to the constructor arguments and using an async static Connect/Create member and marking the constructor internal (as suggested by the initial comment)
| let stepResponse = response.Step | ||
| checkNullAndWarnings response stepResponse | ||
| return response.Status } | ||
| clientSock.ConnectAsync(fullAddress, tok) |> Async.AwaitTask |> Async.RunSynchronously |
There was a problem hiding this comment.
IE move the getConnectedSocket() function call into connect and await it.
| let private applyFieldCheckAndReturnFunction fieldCheck returnFunc (response:SC2APIProtocol.Response) = |
There was a problem hiding this comment.
Can we rename fieldCheck to getResponseField and returnFunc to getResult to match the naming below?
| NewObservation = null | ||
| PlayerId = playerId | ||
| GameInfo = null } | ||
| { |
There was a problem hiding this comment.
I'd prefer the recommended style from https://docs.microsoft.com/en-us/dotnet/fsharp/style-guide/formatting#formatting-record-declarations.
| ) >> Async.Parallel >> Async.RunSynchronously >> List.ofArray >> Result.listFold | ||
|
|
||
| let rec gameLoop playersResult = |
There was a problem hiding this comment.
Maybe its just me but I feel the while loop is a lot more readable. Personally I feel like mutable state is acceptable at this level
| open SC2APIProtocol | ||
|
|
||
| [<EntryPoint>] | ||
| let main argv = | ||
| let userSettings = Sc2SettingsFile.settingsFromUserDir() | ||
| //let userSettings = Sc2SettingsFile.settingsFromUserDir() |
No description provided.