mhowlett / NNanomsg

.NET binding for nanomsg
MIT License
179 stars 52 forks source link

Socket class, not procedural interface #4

Closed mhowlett closed 10 years ago

mhowlett commented 10 years ago

This would be more idiomatic C# and slightly more convenient.

kwpatrick commented 10 years ago

So I'm working through a cleanup pass, and merging the two socket classes. Some discussion topics:

mhowlett commented 10 years ago

On Sun, Oct 13, 2013 at 12:32 AM, MasonOfWords notifications@github.comwrote:

So I'm working through a cleanup pass, and merging the two socket classes. Some discussion topics:

-

Naming - Classes could be unprefixed, prefixed by "Nanomsg", prefixed by "NanoMsg", or prefixed by "NN". The pascal-cased version would be the one most broadly adhering to MS guidelines (although the "Msg" abbreviation is innately a no-no), but I can also see favoring "NN" for both brevity and paralleling the native interface. For reference, the managed sqlite3 library went with "System.Data.SQLite" as a namespace and prefixed public classes with "SQLite".

NanoMsg feels wrong to me, though is as you say is arguably the most idiomatic C# option. Considering Nanomsg to be a new word is also arguably valid and to me feels a little better. I like that NN can be both justified as copying the c method naming convention and also that .net libraries are often called N-something. So if we're called NNanomsg, NN is a good abbreviation to that (and being short is also a plus).

And now you mention it, you are right - it is pretty standard for libraries to prefix public class names with something. So I think I've just argued I think we should use NanomsgSocket or NNSocket. And I would guess your preference is NanoMsgSocket. Finally, of course we could go with no prefix.

I have no clear conviction. Right now NanomsgSocket seems best to me - by a whisker - and NNSocket second best. However my opinion may flip on that, possibly even into the other two options..

-

Native interface - I added a unified Interop class, which will allow 64-bit and 32-bit to coexist in a single AnyCode deployment, as well as taking care of platform differences. Would you be okay if I updated all native usages to this class? I tried to match the pattern that other native library wrappers have established.

Yes, this looks a far superior solution to what I was doing.

-

Exceptions - Your usage seems ideal here. I'll also allow some non-throwing overloads for frequently-called methods, like Send and Receive, to let users have a path that avoids managed allocations. I'll also avoid throwing an exception if the non-blocking method's error is EAGAIN, since that's a part of the semantics of the method and not truly a failure state. I'm also adding a way of letting the nanomsg exceptions add custom error text, so that we can include info about what led to the error.

On having no-throwing overloads: Absolutely. I think we need the exceptions because it is idiomatic C#, but I reckon i'd use no-throw overloads more.

On EAGAIN handling: Yep, sounds good. I may have an opinion on specifics of the API, or this may be obvious, I haven't thought it through enough.

Custom error messages: was getting to this.

Matt.

— Reply to this email directly or view it on GitHubhttps://github.com/mhowlett/NNanomsg/issues/4#issuecomment-26202030 .