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.

Sign up for GitHub

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

Jump to bottom

Class name improvements #19

Draft
wants to merge 3 commits into
base: 0.1.x
Choose a base branch
from
Draft

Class name improvements #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2466e8f
Failing tests for new aliases
mecha Aug 8, 2023
a0d049c
Add Callback, Instance, and TemplatedStr aliases
mecha Aug 8, 2023
2c5a979
Changelog entry for newly added aliases
mecha Aug 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/)
and this project adheres to [Semantic Versioning](http://semver.org/).

## [[*next-version*]] - YYYY-MM-DD
### Added
- New `Instance` alias for the `Constructor` helper.
- New `Callback` alias for the `FuncService` helper.
- New `TemplatedStr` alias for the `StringService` helper.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just Template, since our context here is rather narrow?

Copy link
Member Author

@mecha mecha Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not so sure it's narrow at all. It's fairly reasonable to assume that a module may want to have actual template services and may need to import a Template class from somewhere else. So hogging the Template name in the module scope would be inconvenient, versus a simple dStr suffix that, IMO, also adds a bit more clarity (since the service helper isn't a reusable template, but is template-ed exactly once).

Copy link
Member

@XedinUnknown XedinUnknown Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The context I'm referring to is the context of this package, which I see as different (helpful, faster, easier, simpler) ways to define services, besides helpers etc. In this context, I don't feel that there's more likely to be another Template class than another Callback class.

In that light, I'm not sure that these new aliases are actually as helpful as I previously thought. Maybe, the convention could be XService, or XDefinition, e.g. ConstructorDefinition and FuncDefinition. By that analogy, it would be StringDefinition, then.

I prefer Definition to Service, because the service is the product of resolving a definition. This makes a very clear distinction. I think we should base the nomenclature on this.

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, that's the exact opposite of what this PR aims to achieve - simple, short, and clear names for definitions to help make modules feel more declarative.

i.e. this:

'foo' => new Instance(Foo::class, ['dep']),

Rather than this:

'foo' => new ConstructorDefinition(Foo::class, ['dep']),

If we're going to make the names even longer with a standard suffix, I'd rather just use bare functions:

'foo' => fn ($c) => new Foo($c->get('dep')),

Copy link
Member

@XedinUnknown XedinUnknown Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You'd rather new Foo($c->get('dep')) than Factory(['dep'], fn (DepType $dep) => new Foo($dep)? I always thought the point of things like Factory was to have a declarative way to provide dependency and type information. If the name of e.g. Factory is a little longer - I don't mind.

At the same time, Dhii\Services\Constructor is in fact not a constructor, but a definition that invokes a constructor. This hints to me that it should be named accordingly, such as ConstructorDefinition (or maybe ConstructorDef, whatever). And a TemplatedString is not a templated string (and StringService is not a service), in the same way.

That said, I recognize that you or me or someone else may want to shorten these names for convenience, speed, readability, or another reason (maybe static analysis?). In that case, since this person would already have e.g. use Dhii\Services\StringDefinition in their file, all they'd have to do to achieve this is add as TemplatedStr to this line. Most of the time, they'd do this only once for every module they want to have such aliases in.

So, to summarize my thoughts:
I recognize both a need to standardize and to potentially shorten these names. The latter can be done by renaming (perhaps temporarily via extension, like you did) some classes. The former can be done on the consumer's side with great ease, which also relieves us of having to make the decisions on which shorter names are better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Naming things what they are (e.g. ConstructorDef) is not good API design IMO. The names are for the consumer, and as a consumer I find the current names annoying. They expose too much details about the underlying system.

As a consumer, all I care about is that, for example, foo is an instance of Foo. I don't care that the definition that I'm creating calls the constructor. I just want to see what my services are, which in this case means "what they get resolved to". Making me think about the resolution itself is just extra IMO.

I though we had agreed on this already, here: #13

Furthermore, pushing the burden of names and aliases on the consumer is not a solution IMO, but an admission of fault: "We couldn't name it properly, so you do it".

Copy link
Member

Choose a reason for hiding this comment

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

Naming things what they are (e.g. ConstructorDef) is not good API design IMO. The names are for the consumer, and as a consumer I find the current names annoying. They expose too much details about the underlying system.

The name of a class should expose details of the underlying system. It's the interface that needs to be generic. So, in this example, there could be a ServiceDefinition interface, and ConstructorDef is a concrete implementation. You need to know that it's Constructor-related because you know that you need to instantiate a service by invoking a constructor.

As a consumer, all I care about is that, for example, foo is an instance of Foo. I don't care that the definition that I'm creating calls the constructor. I just want to see what my services are, which in this case means "what they get resolved to"

See above. If you don't care how the definition will resolve the service, then you cannot use defniniton classes correctly, because you need to know how each one instantiates the service (in our case, by using a constructor).

Furthermore, pushing the burden of names and aliases on the consumer is not a solution IMO, but an admission of fault: "We couldn't name it properly, so you do it".

Like I explained, it's not a fault. Having descriptive names is great, and is already enough. If consumers believe that it's more convenient to write Instance instead of ConstructorDef, then it is up to them to add as Instance to the import. It's the consumer's choice, because we don't and can't know what they feel is best for them, besides what we already have. And since with the convention the nomenclature gets order, while the names are still relatively short and easy to type, it's up to the consumer to re-order that for their specific purpose, of which I cannot know.

I though we had agreed on this already, here: #13

Yep, when I saw it, I didn't think too much of what exactly the names will be (although the ones you suggested didn't raise any alarms), but saw and acknowledged that they could be better, more descriptive. To me, ConstructorDef is more descriptive than Instance, because it tells me of what the thing I am using is, rather than what it resolves to. Specifically, like I wrote, it's necessary to know how the service will be resolved (i.e. via a constructor, or redirection to another service, or string interpolation, or just by returning the passed value) in order to pick the right one. Instance tells me more of what the service is (as you wrote), but this is not something that is in the scope of a service definition. I don't see why a class name would describe something other than what it is and what makes it special/different to others.

Copy link
Member

Choose a reason for hiding this comment

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

I appreciate if you want to address these things by what the services are, rather than the definition classes themselves. But this sounds like a personal preference, and I don't feel that it's something that is necessarily good for the majority of consumers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Understood. Shall I close the PR? We can revisit this topic later.


## [0.1.1-alpha3] - 2023-02-01
### Added
Expand Down
9 changes: 9 additions & 0 deletions src/Factories/Callback.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php

declare(strict_types=1);

namespace Dhii\Services\Factories;

class Callback extends FuncService
{
}
9 changes: 9 additions & 0 deletions src/Factories/Instance.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php

declare(strict_types=1);

namespace Dhii\Services\Factories;

class Instance extends Constructor
{
}
9 changes: 9 additions & 0 deletions src/Factories/TemplatedStr.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php

declare(strict_types=1);

namespace Dhii\Services\Factories;

class TemplatedStr extends StringService
{
}
9 changes: 9 additions & 0 deletions tests/unit/Factories/ConstructorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Dhii\Services\Tests\Unit\Factories;

use Dhii\Services\Factories\Constructor;
use Dhii\Services\Factories\Instance;
use Dhii\Services\Service;
use Dhii\Services\Tests\Helpers\MockContainer;
use Exception;
Expand All @@ -23,6 +24,14 @@ public function testIsService()
static::assertInstanceOf(Service::class, new Constructor(''));
}

/**
* @since [*next-version*]
*/
public function testAlias()
{
static::assertInstanceOf(Constructor::class, new Instance(''));
}

/**
* @since [*next-version*]
*/
Expand Down
12 changes: 12 additions & 0 deletions tests/unit/Factories/FuncServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Dhii\Services\Tests\Unit\Factories;

use Dhii\Services\Factories\Callback;
use Dhii\Services\Factories\FuncService;
use Dhii\Services\Service;
use Dhii\Services\Tests\Helpers\MockContainer;
Expand All @@ -24,6 +25,17 @@ public function testIsService()
static::assertInstanceOf(Service::class, $subject);
}

/**
* @since [*next-version*]
*/
public function testAlias()
{
$subject = new Callback([], function () {
});

static::assertInstanceOf(FuncService::class, $subject);
}

/**
* @since [*next-version*]
*/
Expand Down
9 changes: 9 additions & 0 deletions tests/unit/Factories/StringServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Dhii\Services\Tests\Unit\Factories;

use Dhii\Services\Factories\StringService;
use Dhii\Services\Factories\TemplatedStr;
use Dhii\Services\Service;
use Dhii\Services\Tests\Helpers\MockContainer;
use PHPUnit\Framework\TestCase;
Expand All @@ -21,6 +22,14 @@ public function testIsService()
static::assertInstanceOf(Service::class, new StringService(''));
}

/**
* @since [*next-version*]
*/
public function testAlias()
{
static::assertInstanceOf(StringService::class, new TemplatedStr(''));
}

/**
* @since [*next-version*]
*/
Expand Down
Loading