Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Streams IO docs #3127

Merged
merged 6 commits into from
Oct 2, 2017
Merged

Conversation

alexvaluyskiy
Copy link
Contributor

Fixed: #3109

@alexvaluyskiy alexvaluyskiy changed the title Improve Streams IO docs [WIP] Improve Streams IO docs Sep 26, 2017
Copy link
Contributor

@Horusiath Horusiath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is missing pictures, I think, that in general original doc is missing few things (i.e. BindAndHandle method is very useful IMO, but totally ignored here, similarly JsonFraming).

Also regarding example tests - have you actually tried to connect to a TCP server? I think that those scenarios should be verifiable using i.e. telnet command. This was the reason, why I haven't published this doc earlier.

{

#region echo-server-simple-bind
var tcp = new TcpExt(Sys.AsInstanceOf<ExtendedActorSystem>());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ugly API (it should be hidden IMHO). Sys.TcpStream() looks a lot better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok :)

@alexvaluyskiy
Copy link
Contributor Author

@Horusiath We also missed .NET Streams (MemoryStream, FileStream, etc)

@alexvaluyskiy
Copy link
Contributor Author

@Horusiath I've added images added an example with BindAndHandle. And have tested the first example with netcat on Linux Subsystem for Windows.

Still working on the rest two examples

@@ -144,6 +145,7 @@ public class TcpExt : IExtension
/// TBD
/// </summary>
/// <param name="system">TBD</param>
[InternalApi]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made it internal


We can then test the TCP server by sending data to the TCP Socket using `netcat` (on Windows it is possible to use Linux Subsystem for Windows):
```
echo -n "Hello World" | netcat 127.0.0.1 8888
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

netcat is not available on Windows by default (or by Git bash). Have you tried to use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes


string ReadLine(string prompt)
{
// TODO: implement it
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have no idea how to convert it to C#

val input = new AtomicReference("Hello world" :: "What a lovely day" :: Nil)
def readLine(prompt: String): String = {
    input.get() match {
    case all @ cmd :: tail if input.compareAndSet(all, tail) => cmd
    case _ => "q"
    }
}

Copy link
Contributor

@Horusiath Horusiath Sep 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very convoluted way of having concurrent queue with 2 elements in it ("Hello world", "What a lovely day") and dequeueing those elements in thread safe fashion. Once a readLine hits an empty queue a "q" string is returned instead of normal content.

More or less the code would be:

var input = new ConcurrentQueue<string>(new[] { "Hello world", "What a lovely day" });
string ReadLine(string prompt) => input.TryDequeue(out var cmd) ? cmd : "q";

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, It works now

@Horusiath
Copy link
Contributor

Approvals API needs to be updated.

@alexvaluyskiy alexvaluyskiy changed the title [WIP] Improve Streams IO docs Improve Streams IO docs Sep 30, 2017
@Aaronontheweb
Copy link
Member

still missing API approval

@Aaronontheweb Aaronontheweb merged commit 67bced3 into akkadotnet:dev Oct 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants