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

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

wants to merge 3 commits into from

Conversation

mecha
Copy link
Member

@mecha mecha commented Aug 8, 2023

Adds more human-friendly aliases for the following service helper classes:

  • Instance -> Constructor
  • Callback -> FuncService
  • TemplatedStr -> StringService

@mecha mecha added the enhancement New feature or request label Aug 8, 2023
@mecha mecha requested a review from XedinUnknown August 8, 2023 17:16
@mecha mecha self-assigned this Aug 8, 2023
### 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

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

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

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.

Copy link
Member

@XedinUnknown XedinUnknown left a comment

Choose a reason for hiding this comment

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

I was going to suggest class_alias(), but then realized that we may find the extra layer which comes with inheritance - helpful while we transition.

@mecha
Copy link
Member Author

mecha commented Aug 9, 2023

I wanted to avoid aliases since I've had some issues in the past with editors and LSPs not treating class aliases the same as classes in all cases, which can be frustrating.

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

Successfully merging this pull request may close these issues.

2 participants