Stream data to export file

[DanielSpindler83]

Please note this PR uses a branch based off the branch unit-testing also used in #15
I reworked the pingsession.cs to be stateful - it is only passed a single ping request.
Removed the need for a list of PingRequests.
Moved the PingRequestExportModel class into Pingalot.Core
Added Export File to the PingRequestOptions - maybe not the best idea but was the easiest to get the file details passed through to the PingRequestAgent.
Added the File Export function into PingRequestAgent - export a record at a time as it occurs.
Catered for an error with the export file path - if error then create file local to the exe with a generic name plus datetime.
Added a final ping stats method to pingstats to just pass in the end time.
The memory is now constant for the life of the program as we don't keep a list of ping requests.
Updated existing unit-tests to ensure all passing.

[codecov]

Codecov Report

Merging #16 (4adf00f) into main (4ae287e) will increase coverage by 12.66%.
The diff coverage is 48.93%.

❗ Current head 4adf00f differs from pull request most recent head 5ea55e7. Consider uploading reports for the commit 5ea55e7 to get more accurate results

@@             Coverage Diff             @@
##             main      #16       +/-   ##
===========================================
+ Coverage   30.76%   43.43%   +12.66%     
===========================================
  Files           2        3        +1     
  Lines          65       99       +34     
  Branches       10        9        -1     
===========================================
+ Hits           20       43       +23     
- Misses         43       56       +13     
+ Partials        2        0        -2     

Impacted Files	Coverage Δ
src/Pingalot.Core/PingRequestAgent.cs	0.00% <0.00%> (ø)
src/Pingalot.Core/PingRequestExportModel.cs	0.00% <0.00%> (ø)
src/Pingalot.Core/PingSession.cs	91.48% <85.18%> (+43.87%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ae287e...5ea55e7. Read the comment docs.

[Turnerj]

Just FYI - I haven't forgotten about this, just balancing a lot of plates at the moment. Will get back to this sometime this week. 🙂

[DanielSpindler83]

np, i understand, I got a bit busy myself too. Thanks for the heads up.

…

On Sun, Jun 26, 2022 at 12:07 PM James Turner ***@***.***> wrote: Just FYI - I haven't forgotten about this, just balancing a lot of plates at the moment. Will get back to this sometime this week. 🙂 — Reply to this email directly, view it on GitHub <#16 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHGYXCPVHWDAAHIFUSUZUXDVQ63MVANCNFSM5WX3AJIQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[DanielSpindler83]

Hi @Turnerj , here is my latest version of single file stream writer.
Whilst this is working, I have a dirty feeling this is far from elegant.

I have added a new command line switch (-e) as a flag to use a default export file path and name alongside the exe.
If an export path is provided, with or without -e, then the provided path is used.
To be brutally honest, i spent a LONG time trying to work out the whole export \ default thingy.

I refactored the main workload into a "PerformPings" method so I could conditionally wrap the single stream around it.
I have reservations about how this is done - i pass in a null csv writer for when we don't have an export file.....more than likely a better way.
If we don't get provided a path at all, then we run PerformPings without the using file steam wrappers.

Go nuts and poke some holes in this , as I'm sure its terrible in more ways than one.
I appreciate your time.

Thanks

[Turnerj]

I think we can flip the code around a bit - rather than trying to pass the exporting logic in, we instead make it listen on the same event we are pushing data out by. Basically when we know we want to export, we create a new instance of some exporter class that manages the file access and have it listen on our PingCompleted event. Every time the event is raised, we write the record to disk.

[DanielSpindler83]

I think we can flip the code around a bit - rather than trying to pass the exporting logic in, we instead make it listen on the same event we are pushing data out by. Basically when we know we want to export, we create a new instance of some exporter class that manages the file access and have it listen on our PingCompleted event. Every time the event is raised, we write the record to disk.

I can have a go at that.....

[DanielSpindler83]

I think we can flip the code around a bit - rather than trying to pass the exporting logic in, we instead make it listen on the same event we are pushing data out by. Basically when we know we want to export, we create a new instance of some exporter class that manages the file access and have it listen on our PingCompleted event. Every time the event is raised, we write the record to disk.

So I have a new file exporter class and I have setup a method to listen to the PingCompleted event. Im thinking we can no longer use a single set of using statements as the event leaves that code block, then comes back and the filestream objects get cleaned up - I get "object reference not set to an instance of an object"...exception. I am thinking we dont use the using statements and have the fileexporter dispose of the streams once the instance of the fileexporter is no longer used - with the file steams as member of the file exporter....
Is this the correct approach?

[Turnerj]

I am thinking we dont use the using statements and have the fileexporter dispose of the streams once the instance of the fileexporter is no longer used - with the file steams as member of the file exporter....

Yeah, pretty much that. We make the file exporter disposable (with a using statement etc) and make that dispose of our steam when it disposes.

[DanielSpindler83]

@Turnerj working version using an event to conduct the export.
I did experiment with a few different versions, however i feel i make things overly complex and borderline unreadable.
If i come back to it later on and I cant read my own code then its poor and i discard it.
Hopefully this is nice and simple.
Couldn't work out how to wrap my new PingRequestFileExporter in a using statement.

[Turnerj]

OK, I see why you had trouble trying to dispose of it. A quick-and-dirty workaround would be to declare PingRequestFileExporter? pingRequestFileExporter = null; before the if-statement and then wrap the rest of the code in a try/finally. Then in the finally, we go pingRequestFileExporter?.Dispose();.

For a more complete solution, it might be a more ground-up redesign of how the classes interact with each other so we don't even need the file exporter within the specific visual implementations.

[Turnerj]

What I mean by the last part is that some of the difficulty is really just how the classes call each other. Ideal world would have something like each "layout" just register events rather than handle the life cycle itself. The ping session is kinda redundant now as we don't do anything with it. Remove the replication from the cancel key press etc logic in each layout.

If I were to name what I'm thinking, something like "pipelines". Each thing (a layout, exporting, etc) is just an item on the pipeline. I guess kinda like how the middleware works for ASP.NET Core but more around events than just calling a next(); action.