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

Fix interface naming conventions before 1.0? #633

Closed
nvivo opened this issue Feb 16, 2015 · 11 comments
Closed

Fix interface naming conventions before 1.0? #633

nvivo opened this issue Feb 16, 2015 · 11 comments

Comments

@nvivo
Copy link
Contributor

nvivo commented Feb 16, 2015

Are there any plans to standardize interface names before 1.0?

It feels strange to use interfaces without an I prefix in .NET. I think it would help the project to have the port using .NET conventions. Just as properties from java are converted from getContext to Context, an LoggingAdapter feels more natural in .NET as ILoggingAdapter.

I saw this was quickly discussed before here but was probably forgotten.

Scanning the project, I found the following interfaces that don't use the .NET naming conventions.

  • Akka.Actor.ActorRefProvider, Akka (Public)
  • Akka.Actor.ActorRefScope, Akka (Public)
  • Akka.Actor.AutoReceivedMessage, Akka (Public)
  • Akka.Actor.Cell, Akka (Public)
  • Akka.Actor.Inboxable, Akka (Public)
  • Akka.Actor.IndirectActorProducer, Akka (Public)
  • Akka.Actor.Internal.ChildrenContainer, Akka (Public)
  • Akka.Actor.Internal.ChildStats, Akka (Public)
  • Akka.Actor.Internal.InternalSupportsTestFSMRef2, Akka` (Public)
  • Akka.Actor.Internal.SuspendReason+WaitingForChildren, Akka
  • Akka.Actor.Internals.InitializableActor, Akka (Public)
  • Akka.Actor.LocalRef, Akka
  • Akka.Actor.LoggingFSM, Akka (Public)
  • Akka.Actor.NoSerializationVerificationNeeded, Akka (Public)
  • Akka.Actor.PossiblyHarmful, Akka (Public)
  • Akka.Actor.RepointableRef, Akka (Public)
  • Akka.Actor.WithBoundedStash, Akka (Public)
  • Akka.Actor.WithUnboundedStash, Akka (Public)
  • Akka.Dispatch.BlockingMessageQueueSemantics, Akka (Public)
  • Akka.Dispatch.BoundedDequeBasedMessageQueueSemantics, Akka (Public)
  • Akka.Dispatch.BoundedMessageQueueSemantics, Akka (Public)
  • Akka.Dispatch.DequeBasedMailbox, Akka (Public)
  • Akka.Dispatch.DequeBasedMessageQueueSemantics, Akka (Public)
  • Akka.Dispatch.MessageQueues.MessageQueue, Akka (Public)
  • Akka.Dispatch.MultipleConsumerSemantics, Akka (Public)
  • Akka.Dispatch.RequiresMessageQueue1, Akka` (Public)
  • Akka.Dispatch.Semantics, Akka (Public)
  • Akka.Dispatch.SysMsg.SystemMessage, Akka (Public)
  • Akka.Dispatch.UnboundedDequeBasedMessageQueueSemantics, Akka (Public)
  • Akka.Dispatch.UnboundedMessageQueueSemantics, Akka (Public)
  • Akka.Event.LoggingAdapter, Akka (Public)
  • Akka.FluentConfigInternals, Akka (Public)
  • Akka.Remote.InboundMessageDispatcher, Akka.Remote
  • Akka.Remote.RemoteRef, Akka.Remote
  • Akka.Routing.ConsistentHashable, Akka (Public)

Most issues are in Akka.dll. For reference, there are 65 public interfaces in Akka.dll, 32 use non-standard naming conventions.

@rogeralsing
Copy link
Contributor

I'm personally not a fan of the "I" prefix.
It's a heritage from Hungarian notation and adds nothing of value imo.
why not C for class? S for struct, E for enum? etc.
We clearly don't think that would be a good idea, so why the special treatment for interfaces?

I know that others disagree, but that is my take on it :-)

cc @akkadotnet/developers thoughts?

@panmanphil
Copy link

I do agree. But the alternative is often not that palatable either. In java the convention is the Impl suffix so you don't end up with the interface and implementer having the same name. Only an issue when you have to refer to the interface and an implementor in the same source file of course.

@skotzko
Copy link
Contributor

skotzko commented Feb 16, 2015

My thought is that we should stick w/ the .NET convention here. The actor model and a whole new framework is already a bit mind-bending, so one less point of confusion will aid in its adoption.

@rogeralsing
Copy link
Contributor

@panmanphil ey nice to see you here :-)

@nvivo
Copy link
Contributor Author

nvivo commented Feb 16, 2015

we should stick w/ the .NET convention here. The actor model and a whole new framework is already a bit mind-bending, so one less point of confusion will aid in its adoption.

My point exactly @skotzko. It may not be perfect, but it's the convention and is already used everywhere. I think this is more important in an OSS project. For private/internal stuff I don't mind, but for public types we use directly it's better to follow the standards.

Even if you'd stick with a different naming convention than the rest of the .NET world, still has the issue that half of the public API is different than the other.

@rogeralsing
Copy link
Contributor

I know this has to be done, this is just me waving my arms and showing I don't like it :-)
But yes, lowering any friction for adoption is probably a good thing..

@Aaronontheweb
Copy link
Member

I know this has to be done, this is just me waving my arms and showing I don't like it :-)

Pretty much how I feel too :p

Some of these things are internals that shouldn't be accessible to an end-user to begin with, which is its own issue (need to get more liberal with the internal keyword.)

As for the rest... Some of the things we ported from Akka / Scala that don't translate well into C# - the notion of Scala traits, for instance. Because we don't have multiple / trait inheritance in C#, we've used the marker interface pattern to signal to the internally that an Actor decorated with WithBoundedStash needs a bounded stash.

Does changing the name of WithBoundedStash to IWithBoundedStash make what this interface does more inherently understandable to the end-user? Probably not. The end-user still has to know that this is a weird interface that causes some reflection magic to happen inside the actor construction pipeline, which is the real learning overhead.

We should probably do this anyway though, just so end-users know what's an interface versus a class. I don't think it's going to make it much easier for a new user to understand what all of this stuff does, but there aren't any really good reasons for not doing it.

@nvivo
Copy link
Contributor Author

nvivo commented Feb 16, 2015

Maybe for some things like markers, an attribute would be better suited then? I don't think its necessarily better, its slightly more complicated to check for than an "is TypeName", but it's an option. I'm just thinking out loud here.

I don't think the naming convention makes all the difference in the learning curve, but it certainly makes a difference in the WTF count when someone learning looks at the samples. =) I for one was puzzled with why LoggingAdapter was showing in a different color in my VS at first.

As you said, probably adding some internal keywords would reduce this list to half or a third.

@Aaronontheweb
Copy link
Member

@nvivo ultimately this is something we need to do - kind of have our hands forced by the established convention and the fact that most .NET tooling insists on it. No point in fighting it.

Best way to begin this process is probably to create an issue for marking members as internal (anything in the .Internals namespace, for starters) and clean that up first, then move onto the user-facing stuff.

@SeanKilleen
Copy link
Contributor

Hi all,

I did some house-keeping and created related issues that I think match with the consensus here.

Aaronontheweb added a commit to Aaronontheweb/akka.net that referenced this issue Mar 3, 2015
helps resolve akkadotnet#673

Reimplemented Murmur3, ConsistentHashRouting to use virtual nodes
Added deploy and routerconfig fallback support
Rewrote ActorOf method to LocalActorRefProvider to match Akka
Rewrote ActorOf method to RemoteActorRefProvider to match Akka
Breaking change - renamed ConsistentHashable interface to IConsistentHashable (per akkadotnet#633)
Added MultiNodeTests for ClusterConsistentHashRouting
Implemented Pool routers for Akka.Cluster
@sean-gilliam
Copy link
Member

This issue can be closed since the changes have since been made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants