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

Decide on interface name conventions #8293

Closed
ewinslow opened this Issue May 11, 2015 · 19 comments

Comments

Projects
None yet
4 participants
@ewinslow
Member

ewinslow commented May 11, 2015

Options:

  1. NameInterface (@hypeJunction, @beck24)
  2. IName
  3. Name (@ewinslow, @mrclay, @juho-jaakkola)
@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction
Contributor

hypeJunction commented May 11, 2015

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow May 11, 2015

Member

@Elgg/owners

We need to get this resolved once and for all so we can move forward on some PRs.

My opinion is that interfaces are the only identifiers that get to be excused from containing some implementation details in their name.

Class names should contain some implementation details because that is what classes are: implementations.

I contend that if we name interfaces like "NameInterface" or "IName" then we will encourage concrete classes with undescriptive names like "Name" just because they're the "first" or "default" implementation of an interface.

Member

ewinslow commented May 11, 2015

@Elgg/owners

We need to get this resolved once and for all so we can move forward on some PRs.

My opinion is that interfaces are the only identifiers that get to be excused from containing some implementation details in their name.

Class names should contain some implementation details because that is what classes are: implementations.

I contend that if we name interfaces like "NameInterface" or "IName" then we will encourage concrete classes with undescriptive names like "Name" just because they're the "first" or "default" implementation of an interface.

@beck24

This comment has been minimized.

Show comment
Hide comment
@beck24

beck24 May 11, 2015

Member

I vote for 1

Member

beck24 commented May 11, 2015

I vote for 1

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow May 11, 2015

Member

@hypeJunction @beck24 any particular reasoning?

Member

ewinslow commented May 11, 2015

@hypeJunction @beck24 any particular reasoning?

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction May 11, 2015

Contributor

It looks like camelCase with an l (L) in front. Plus it's easier to read
and register with your eyes
On May 11, 2015 11:07 AM, "Evan Winslow" notifications@github.com wrote:

@hypeJunction https://github.com/hypeJunction @beck24
https://github.com/beck24 any particular reasoning?


Reply to this email directly or view it on GitHub
#8293 (comment).

Contributor

hypeJunction commented May 11, 2015

It looks like camelCase with an l (L) in front. Plus it's easier to read
and register with your eyes
On May 11, 2015 11:07 AM, "Evan Winslow" notifications@github.com wrote:

@hypeJunction https://github.com/hypeJunction @beck24
https://github.com/beck24 any particular reasoning?


Reply to this email directly or view it on GitHub
#8293 (comment).

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow May 11, 2015

Member

I'm thinking of the case where we have plugins using DI and type hinting on our interfaces.

With option 1, we end up with this:

class SomeService {
  public function __construct(ConfigInterface $config, DbInterface $db, InputInterface $input) {}
}

Personally I'd much rather have folks writing this:

class SomeService {
  public function __construct(Config $config, Db $db, Input $input) {}
}

I don't understand your point "It looks like camelCase." What makes the Interface suffix easier than no suffix at all?

Thanks for disussing this rather insignificant but oh-so-important issue of naming conventions (Naming things is the hardest thing in computer science...).

Member

ewinslow commented May 11, 2015

I'm thinking of the case where we have plugins using DI and type hinting on our interfaces.

With option 1, we end up with this:

class SomeService {
  public function __construct(ConfigInterface $config, DbInterface $db, InputInterface $input) {}
}

Personally I'd much rather have folks writing this:

class SomeService {
  public function __construct(Config $config, Db $db, Input $input) {}
}

I don't understand your point "It looks like camelCase." What makes the Interface suffix easier than no suffix at all?

Thanks for disussing this rather insignificant but oh-so-important issue of naming conventions (Naming things is the hardest thing in computer science...).

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow May 11, 2015

Member

Strong arguments in favor of the suffix:

  • PSR uses Interface suffix
  • Symfony2 standards require Interface suffix

But I'd still like to know why the suffix is desirable in the first place.

Member

ewinslow commented May 11, 2015

Strong arguments in favor of the suffix:

  • PSR uses Interface suffix
  • Symfony2 standards require Interface suffix

But I'd still like to know why the suffix is desirable in the first place.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction May 11, 2015

Contributor

Option 2 looks like camelCase that start with an L in lower case, so it's hard to read.
Option 3 is fine to me, but I imagine we are going to end up with abstract/concrete classes inheriting the interface name, so it will be confusing when class Config implements Config. I suppose, if we are going to use a namespace for interfaces, option 3 should be acceptable with class Config implements Interfaces\Config

Contributor

hypeJunction commented May 11, 2015

Option 2 looks like camelCase that start with an L in lower case, so it's hard to read.
Option 3 is fine to me, but I imagine we are going to end up with abstract/concrete classes inheriting the interface name, so it will be confusing when class Config implements Config. I suppose, if we are going to use a namespace for interfaces, option 3 should be acceptable with class Config implements Interfaces\Config

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow May 11, 2015

Member

I guess I'd argue that this conflict is positive because it encourages us to come up with more descriptive names like InMemoryConfig, SqlBackedConfig, MixedBackendConfig, etc. I.e. describe the implementation somewhat in the name of the class doing the implementing.

Member

ewinslow commented May 11, 2015

I guess I'd argue that this conflict is positive because it encourages us to come up with more descriptive names like InMemoryConfig, SqlBackedConfig, MixedBackendConfig, etc. I.e. describe the implementation somewhat in the name of the class doing the implementing.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay May 11, 2015

Member

I've switched my vote to Name for the simplicity in user code. In practice we can use something like use Elgg\Services\Config as ConfigInterface in our concrete classes to disambiguate, or just list the whole interface name.

Aside: I think it's important in the ServiceProvider to declare concrete classes in the @property-reads because internal Elgg core may need to call non-interface methods and we want IDEs to recognize them. The Application properties can list the interfaces.

Member

mrclay commented May 11, 2015

I've switched my vote to Name for the simplicity in user code. In practice we can use something like use Elgg\Services\Config as ConfigInterface in our concrete classes to disambiguate, or just list the whole interface name.

Aside: I think it's important in the ServiceProvider to declare concrete classes in the @property-reads because internal Elgg core may need to call non-interface methods and we want IDEs to recognize them. The Application properties can list the interfaces.

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow May 12, 2015

Member

@brettp @Srokap @jdalsem Any more thoughts/reasoning to consider?

Member

ewinslow commented May 12, 2015

@brettp @Srokap @jdalsem Any more thoughts/reasoning to consider?

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay May 12, 2015

Member

Should be noted that PSR-2 doesn't standardize "FooInterface", that's just an example valid class name. Also the massively popular Laravel uses "Vendor\Contracts\Foo".

I'm going to push forward with Elgg\Services\Name. Fairly easy to change later.

Member

mrclay commented May 12, 2015

Should be noted that PSR-2 doesn't standardize "FooInterface", that's just an example valid class name. Also the massively popular Laravel uses "Vendor\Contracts\Foo".

I'm going to push forward with Elgg\Services\Name. Fairly easy to change later.

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow May 15, 2015

Member

We have a majority listed in favor of option 3, and @hypeJunction has said he's ok with it, too.

If we don't hear from folks in 3 more days I think we will just consider "no suffix" to be the standard.

Member

ewinslow commented May 15, 2015

We have a majority listed in favor of option 3, and @hypeJunction has said he's ok with it, too.

If we don't hear from folks in 3 more days I think we will just consider "no suffix" to be the standard.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay May 15, 2015

Member

FYI I've already merged fac589b. If we wanna change it I'm happy to help.

Member

mrclay commented May 15, 2015

FYI I've already merged fac589b. If we wanna change it I'm happy to help.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction May 15, 2015

Contributor

I was looking through some of the existing API and we already use option 3
for some interfaces, e.g. Queue. I suppose we can just continue doing so.
On May 15, 2015 1:22 PM, "Steve Clay" notifications@github.com wrote:

FYI I've already merged fac589b
fac589b.
If we wanna change it I'm happy to help.


Reply to this email directly or view it on GitHub
#8293 (comment).

Contributor

hypeJunction commented May 15, 2015

I was looking through some of the existing API and we already use option 3
for some interfaces, e.g. Queue. I suppose we can just continue doing so.
On May 15, 2015 1:22 PM, "Steve Clay" notifications@github.com wrote:

FYI I've already merged fac589b
fac589b.
If we wanna change it I'm happy to help.


Reply to this email directly or view it on GitHub
#8293 (comment).

@beck24

This comment has been minimized.

Show comment
Hide comment
@beck24

beck24 May 15, 2015

Member

I'm not strongly opinionated on it either way, what we have is fine

Member

beck24 commented May 15, 2015

I'm not strongly opinionated on it either way, what we have is fine

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow May 16, 2015

Member

Sounds like this is decided then. Would be good to document it in our coding standards before we close this ticket.

Member

ewinslow commented May 16, 2015

Sounds like this is decided then. Would be good to document it in our coding standards before we close this ticket.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay May 26, 2015

Member

I'm starting to doubt this decision. I'm working on several PRs that expose interfaces and it's getting confusing to tell between, e.g., \Elgg\Services\AjaxApi\Response (interface devs should typehint on) and \Elgg\AjaxApi\Response (concrete object passed to handler). Further, the "Services" namespace works for services, but most of our interfaces are probably not going to be services. I'm going back to preferring the simple \Elgg\ConfigInterface and \Elgg\AjaxApi\ResponseInterface, and with the interfaces sitting right next to the implementations. Next best thing would be a "Contracts" namespace for all interfaces.

Member

mrclay commented May 26, 2015

I'm starting to doubt this decision. I'm working on several PRs that expose interfaces and it's getting confusing to tell between, e.g., \Elgg\Services\AjaxApi\Response (interface devs should typehint on) and \Elgg\AjaxApi\Response (concrete object passed to handler). Further, the "Services" namespace works for services, but most of our interfaces are probably not going to be services. I'm going back to preferring the simple \Elgg\ConfigInterface and \Elgg\AjaxApi\ResponseInterface, and with the interfaces sitting right next to the implementations. Next best thing would be a "Contracts" namespace for all interfaces.

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow May 26, 2015

Member

This tells me we need to pick more descriptive names for the concrete
implementations.

On Mon, May 25, 2015, 6:07 PM Steve Clay notifications@github.com wrote:

I'm starting to doubt this decision. I'm working on several PRs that
expose interfaces and it's getting confusing to tell between, e.g.,
\Elgg\Services\AjaxApi\Response (interface) and \Elgg\AjaxApi\Response
(concrete object passed to handler). Further, the "Services" namespace
works for services, but most of our interfaces are probably not going to be
services. I'm going back to preferring the simple \Elgg\ConfigInterface and
\Elgg\AjaxApi\ResponseInterface, so the interfaces sit right next to the
implementations.


Reply to this email directly or view it on GitHub
#8293 (comment).

Member

ewinslow commented May 26, 2015

This tells me we need to pick more descriptive names for the concrete
implementations.

On Mon, May 25, 2015, 6:07 PM Steve Clay notifications@github.com wrote:

I'm starting to doubt this decision. I'm working on several PRs that
expose interfaces and it's getting confusing to tell between, e.g.,
\Elgg\Services\AjaxApi\Response (interface) and \Elgg\AjaxApi\Response
(concrete object passed to handler). Further, the "Services" namespace
works for services, but most of our interfaces are probably not going to be
services. I'm going back to preferring the simple \Elgg\ConfigInterface and
\Elgg\AjaxApi\ResponseInterface, so the interfaces sit right next to the
implementations.


Reply to this email directly or view it on GitHub
#8293 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment