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

Service definition #78

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Service definition #78

wants to merge 13 commits into from

Conversation

xepozz
Copy link
Member

@xepozz xepozz commented Jul 29, 2023

Q A
Is bugfix?
New feature? ✔️
Breaks BC?
Fixed issues

I'd like to present a new way to configure services:

$b = ServiceDefinition::for(Phone::class)
        ->constructor(['name' => 'Retro', 'version' => '1.0'])
        ->set('dev', true)
        ->set('codeName', 'b')
        ->call('setId', [42])
        ->call('setColors', ['yellow']);

It's the same as the configuration with ArrayDefinition:

$b = ArrayDefinition::fromConfig([
    ArrayDefinition::CLASS_NAME => Phone::class,
    ArrayDefinition::CONSTRUCTOR => ['version' => '2.0'],
    '$dev' => true,
    '$codeName' => 'b',
    'setId()' => [42],
    'setColors()' => ['yellow'],
]);

@xepozz xepozz requested a review from a team July 29, 2023 10:28
@codecov
Copy link

codecov bot commented Jul 29, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (4c97eb0) 100.00% compared to head (e1cdb16) 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##              master       #78   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       242       260   +18     
===========================================
  Files             16        17    +1     
  Lines            664       712   +48     
===========================================
+ Hits             664       712   +48     
Files Changed Coverage Δ
src/ServiceDefinition.php 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@what-the-diff
Copy link

what-the-diff bot commented Jul 29, 2023

PR Summary

  • New file for Service Definition
    A new file titled ServiceDefinition.php has been added. This file contains the implementation of a service definition class, which is crucial for defining the different services our application can offer and how they interact.

@vjik
Copy link
Member

vjik commented Jul 29, 2023

It is more correct to compare variants:

// New
ServiceDefinition::for(Phone::class)
        ->constructor(['name' => 'Retro', 'version' => '1.0'])
        ->setProperty('$dev', true)
        ->setProperty('$codeName', 'b')
        ->callMethod('setId()', [42])
        ->callMethod('setColors()', ['yellow']);

// Current		
[
    'class' => Phone::class,
    '__construct()' => ['version' => '2.0'],
    '$dev' => true,
    '$codeName' => 'b',
    'setId()' => [42],
    'setColors()' => ['yellow'],
]

For me new variant gives one benefit only:

  • autocomplete for constructor and class keys.

Everything else is redundant and only complicates code:

  • keys $dev, setId() are understandable by themselves, new syntax added redundant code in the form of methods setProperty() and callMethod(),
  • bulk usage ServiceDefinition::for([...]) is greatly litter configuration and it will distract attention when reading configuration.

My opinion is keep current syntax and do not add ServiceDefinition.

@samdark
Copy link
Member

samdark commented Aug 18, 2023

@xepozz please check tests.

@samdark
Copy link
Member

samdark commented Aug 26, 2023

// New
ServiceDefinition::for(Phone::class)
        ->constructor(['name' => 'Retro', 'version' => '1.0'])
        ->set('dev', true)
        ->set('codeName', 'b')
        ->call('setId', [42])
        ->call('setColors', ['yellow']);

// Current		
[
    'class' => Phone::class,
    '__construct()' => ['version' => '2.0'],
    '$dev' => true,
    '$codeName' => 'b',
    'setId()' => [42],
    'setColors()' => ['yellow'],
]

@samdark
Copy link
Member

samdark commented Aug 26, 2023

While the syntax is alright, it adds another way to do the same thing which I'm not sure it a good thing.

@Tigrov
Copy link
Member

Tigrov commented Aug 26, 2023

The syntax is great. But as already noted:

  1. another way to do what is already there
  2. write more characters set, call instead of $, ().

But why not, if somebody likes to write the config in this way. Does it work faster?

@xepozz
Copy link
Member Author

xepozz commented Oct 4, 2023

image image

Comparing these two options I like more the first one

@xepozz xepozz changed the title [PoC] Service definition Service definition Oct 4, 2023
@vjik
Copy link
Member

vjik commented Oct 4, 2023

image image
Comparing these two options I like more the first one

It's incorrect comparison. Correct so:

// New
ServiceDefinition::for(Phone::class)
        ->constructor(['version' => '1.0'])
        ->set('dev', true)
        ->set('codeName', 'b')
        ->call('setId', [42])
        ->call('setColors', ['yellow']);

// Current		
[
    'class' => Phone::class,
    '__construct()' => ['version' => '2.0'],
    '$dev' => true,
    '$codeName' => 'b',
    'setId()' => [42],
    'setColors()' => ['yellow'],
]

Also appear problem with configuration merge. With objects it is more difficult in implementation.

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

Successfully merging this pull request may close these issues.

5 participants