Skip to content

Conversation

@xepozz
Copy link
Member

@xepozz xepozz commented Apr 19, 2022

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

Added ability to use iterable parameters.
For example, now you cannot use Generator as tags values.

@xepozz xepozz requested a review from a team April 19, 2022 19:53
@xepozz xepozz added the status:code review The pull request needs review. label Apr 19, 2022
@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (8e264ff) to head (0e4059d).
Report is 38 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##              master      #299   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       158       160    +2     
===========================================
  Files             11        11           
  Lines            460       467    +7     
===========================================
+ Hits             460       467    +7     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@vjik vjik left a comment

Choose a reason for hiding this comment

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

Overall good. One change is needed to satisfy psalm.

Co-authored-by: Sergei Predvoditelev <[email protected]>
@samdark
Copy link
Member

samdark commented Dec 5, 2022

@xepozz need a line for CHANGELOG. Also, it looks like BC-breaking change.

@xepozz
Copy link
Member Author

xepozz commented Jul 1, 2023

@xepozz need a line for CHANGELOG. Also, it looks like BC-breaking change.

Done. I can keep it for 2.0

return [
[
'/^Invalid tags configuration: tag should be string, 42 given\.$/',
'Invalid tags configuration: tag should be string, array given.',
Copy link
Member

Choose a reason for hiding this comment

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

That does not look right. The problem here is 42, right?

}
/** @var string $id */

$id = (string) $id;
Copy link
Member

Choose a reason for hiding this comment

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

Not need type-casting. Above check that $id is string.

* @throws InvalidConfigException
*/
private function validateMeta(array $meta): void
private function validateMeta(iterable $meta): void
Copy link
Member

Choose a reason for hiding this comment

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

validateMeta always receive array. Not need iterable.

/** @var mixed $value */
/** @psalm-suppress MixedAssignment */
foreach ($meta as $key => $value) {
$key = (string)$key;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$key = (string)$key;

Not need.

}

$tags = $this->tags[$tag];
$tags = $tags instanceof Traversable ? iterator_to_array($tags, true) : $tags;
Copy link
Member

Choose a reason for hiding this comment

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

Better convert iterable to array before foreach

* and a corresponding value is callable doing the job.
*/
public function getExtensions(): array;
public function getExtensions(): iterable;
Copy link
Member

Choose a reason for hiding this comment

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

Need fix phpdoc.

* an interface name), and a corresponding value is a service definition.
*/
public function getDefinitions(): array;
public function getDefinitions(): iterable;
Copy link
Member

Choose a reason for hiding this comment

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

Need fix phpdoc.

sprintf(
'Invalid tags configuration: tag should be string, %s given.',
$tag
get_debug_type($services)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
get_debug_type($services)
get_debug_type($tag)

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

Labels

status:code review The pull request needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants